[2.0.x] AVR: Atomic bit set and clear of upper pin ports without critical section

[ejtagle]

On AVR, writing to the read register of a port allows to perform an atomic pin toggling without using a critical section.

This saves 3 cycles per access to those upper ports and reduces interrupt processing latency, improves speed and reduces program size.

Also simplified pin toggling for all ports (no need to read before writing as it was done before)

Quoting AVR datasheet:

13.2.2 Toggling the Pin:
Writing a logic one to PINxn toggles the value of PORTxn, independent on the value of DDRxn. Note that the SBI instruction can be used to toggle one single bit in a port.

The following was the code that was generated before this PR, and after applying it:

Clear port bit

; Original Sequence
in      r25, SREG           ; Read interrupt flag status    
cli                         ; Disable interrupts
lds     r24, PORT           ; Get port pins status
andi    r24, ~PinMask       ; Clear the bit of the pin we want to set to 0
sts     PORT, r24           ; Set pin values
out     SREG, r25           ; Restore interrupt status

; New Sequence
lds r2, PORT                ; Get port pins status
and r2, PinMask             ; Isolate the status of the pin we want to clear
sts PIN, r2                 ; Toggle that pin status if it was set. The result is that the pin is now clear

Set Port bit

; Original Sequence
in      r25, SREG           ; Read interrupt flag status    
cli                         ; Disable interrupts
lds     r24, PORT           ; Get port pins status
ori     r24, PinMask        ; Set the bit of the pin we want to set to 1
sts     PORT, r24           ; Set pin values
out     SREG, r25           ; Restore interrupt status

; New sequence
lds r2, PORT                ; Get port pins status
com r2                      ; Invert all pin values
and r2, PinMask             ; Isolate the (inverted) status of the pin we want to set
sts PIN, r2                 ; Toggle that pin status if it was clear. The result is that the pin is now set

[thinkyhead]

Bueller? Bueller? I'm serious about the shorter commit messages.
Having to repeatedly edit them for you is really irksome.

[thinkyhead]

I guess this change will only affect platforms that have a Port H and above. Is that correct?
Which AVR platforms have that many ports?

[thinkyhead]

Ah, so this actually toggles the bit? That's interesting!

[thinkyhead]

Does this work ok for all port addresses, both over and under 0x100?

[ejtagle]

@thinkyhead : This time i tried to make commit messages shorter. Sometimes i just don´t know how to resume them even more... :(

Exactly as you said, this will only be used for PORTH onwards. I assume AVR mega 1280 / 2560
The code used for ports A-G was already optimal (it uses cbi / sbi instructions, that are atomic! . But ports from H onwards are outside of the IO range that those instructions allow. That is why they were handled separately, and still it is worthwhile to do so :)

Yes, the pin toggle, according to the datasheet, works for all ports (http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-2549-8-bit-AVR-Microcontroller-ATmega640-1280-1281-2560-2561_datasheet.pdf , section 13.2.2)

And as a matter of fact, the TOGGLE macro was buggy, as it was reading from the INPUT (_RPORT) register, and writing to the INPUT (_RPORT) register. If you want to use a XOR to toggle a bit, you must use the OUTPUT (_WPORT) register.... 😮

[thinkyhead]

i tried to make commit messages shorter. Sometimes i just don´t know

Well, every genius has a blind spot…

The code used for ports A-G was already optimal (it uses cbi / sbi instructions, that are atomic!

And I modded fastio_AVR.h to use more macros from macros.h including CBI/SBI so that will be more obvious going forward.

the TOGGLE macro was buggy

Looks like we dodged a bullet, then!
Almost no code uses TOGGLE, and only the LED_PIN on AVR.

[Roxy-3D]

We may have a small issue with this... Please check out: #10512

[ejtagle]

That, unfortunately, is a avr-gcc bug. It is not being C99 compliant.

I can reproduce the error with the following minimalistic example:

#define DIO4_DDR a
#define DIO4_PIN 10

#undef _BV
#define _BV(b) (1<<(b))
#define SBI(n,b) n |= _BV(b)
#define _SET_OUTPUT(IO)       SBI(DIO##IO##_DDR, DIO##IO##_PIN)
#define SET_OUTPUT(IO)        _SET_OUTPUT( IO )
#define CONTROLLER_FAN_PIN  4 //hello

  uint8_t a = 0;
  SET_OUTPUT(CONTROLLER_FAN_PIN); /*Set pin used for driver cooling fan */

[p3p]

if it is an avr-gcc bug and not Arduino IDE how does it compile on PlatformIO (Travis)?

[ejtagle]

Maybe is dependant on the Arduino AVR-GCC version ? ... To be honest, i had no problems compiling on Arduino, but...

Let´s check Configuration_adv.h
The only defines for PINs that have comments to their right (triggering the problem) are:

CONTROLLER_FAN_PIN (Used to control a fan that cools down the polulu drivers)
CASE_LIGHT_PIN (you know...)
MAX7219_CLK_PIN (debugging)
MAX7219_DIN_PIN (debugging)
MAX7219_LOAD_PIN (debugging)

And all of them are disabled on most configurations, so they don´t trigger the problem.

[ejtagle]

And i tried, and the only one that creates problems seems to be CONTROLLER_FAN_PIN ...
Maybe Travis is not testing a configuration with a controller fan enabled?