MegaCoreX icon indicating copy to clipboard operation
MegaCoreX copied to clipboard

digitalWrite() backwards compatibility problem

Open benwillcocks opened this issue 5 years ago • 3 comments

Line 174 of wiring_digital.c has the comment "old implementation has side effect when pin set as input - pull up is enabled if this function is called. Should we purposely implement this side effect?". Actually, a more important feature of the old implementation of digitalWrite(), which is missing from the new implementation, is that it can be used to set the output register whilst the pin is configured as an input. Here are two situations where this is useful: 1] if a pin is used for bidirectional communication, it can be necessary to change direction from input to output without changing the state of the pin. To achieve this one needs to read the input, set the output register accordingly, and then make the pin an output. 2] if multiple pins (not necessarily on the same port) are commoned for extra drive strength, then one must be careful to avoid the outputs fighting one another. When changing the state it is necessary to make all the pins inputs, then change the output registers, then make all the pins outputs.

benwillcocks avatar Nov 04 '20 00:11 benwillcocks

Hi!

The problem is that the megaAVR-0 series is different from the classic AVRs. the megaAVR-0 has a dedicated pullup register, while the classic AVR does not. This means we have to decide what would happen when a pin is set to input and digitalWrite is ran.

At the moment, it will turn on and off the pullup. It is also possible to do as you mention in 2), actually set the output register, but this will break compatibility with the official Arduino megaavr package, and that's something I don't want to if I don't have to. I suggest you open this issue in the official repo as well, so the Arduino developers can join the discussion.

MCUdude avatar Nov 04 '20 09:11 MCUdude

Huh... eew. Never even thought of that... and unless I'm mistaken, getting the old behavior back would probably save a couple of bytes of flash, and it's trivial to do. I'm going to just go do it in DxCore and megaTinyCore, on the grounds that - next to code that makes greater sacrifices for compatibility with the classic behavior, it's a bug (I have definitely seen code in the wild that relies on this).

I also, ah, have less deference (among other things) for the official megaAVR core than @MCUdude does.

SpenceKonde avatar Dec 15 '20 03:12 SpenceKonde

Worth noting that the analog to this in pinMode() (having INPUT_PULLUP also set the port output state) was unanimously voted off the core in a discussion thread. One guy started complaining, and everyone else posting showed up in the thread with pitchforks and bricks to throw at me, and nobody said anything nice about having pinMode do this) But no complaints about the digitalWrite() behaviour setting he OUT registers when pin is input though, and people seemed to think that was reasonable

SpenceKonde avatar Mar 17 '22 18:03 SpenceKonde

I'm keeping the current implementation as this is how the official Arduino megaavr core still does it, and IIRC, DxCore and megaTinyCore as well

MCUdude avatar Jan 10 '23 20:01 MCUdude

I changed it like I suggested.

People complained.

I changed it back to the way it was.

People stopped complaining.

When I was debating that, the user opinion was unanimously in favor of removing that emulation. None of the people who advocated it or liked it could be found.

Besides, we have a solution for this now. MegaCoreX has pinConfigure too doesn't it? (btw, your new version of that is now in my cores, after considerable difficulty integrating it without either code duplication or breaking compatibility, but it is now in, and takes either a single pre-OR'ed value or a bunch of constants as separate arguments and it or's them together through your methods.

SpenceKonde avatar Jan 10 '23 23:01 SpenceKonde