Wrong PINx usage?

[stefano-zanotti-88]

To toggle a pin, optiboot.c uses an OR mask: https://github.com/Optiboot/optiboot/blob/81bdd404ff70b76fed716a10bbde90149d98e647/optiboot/bootloaders/optiboot/optiboot.c#L1270

However, it seems that in order to toggle a pin you just need to write 1 to the bit you need, and should write 0 to all the others. Actually, by or-ing, you are also toggling all the bits that are currently high (which changes the state of the outputs, and messes with the pullup state of the inputs). This might not cause problems in many cases, but it really depends on what those pins do on a specific board, which is outside of the bootloader's control.

See here: https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-7810-Automotive-Microcontrollers-ATmega328P_Datasheet.pdf Section 13.1:

The port input pins I/O location is read only, while the data register and the
data direction register are read/write. However, writing a logic one to a bit in the PINx register, will result in a toggle in the
corresponding bit in the data register.

I've tried to compile a simple test on an ATmega328PB, and this is the generated asm:

    ;PINB = 1;
    LDI R24,0x01        ; Load immediate
    OUT 0x03,R24        ; Out to I/O location
    ;PINB |= 1;
    SBI 0x03,0      ; Set bit in I/O register
    ;PORTB ^= 1;
    IN R25,0x05     ; In from I/O location
    EOR R24,R25     ; Exclusive OR 
    OUT 0x05,R24        ; Out to I/O location

So it seems that the "wrong" PIN|= is compact, while the proper one PIN= uses more flash, though it still wins over a naive PORT^=

This also depends on the specific PORT/part. On an ATmega2560:

    ;PINL = 1;
    LDI R24,0x01        ; Load immediate
    STS 0x0109,R24      ; Store direct to data space
    ;PINL |= 1;
    LDS R25,0x0109      ; Load direct from data space
    ORI R25,0x01        ; Logical OR with immediate
    STS 0x0109,R25      ; Store direct to data space
    ;PORTL ^= 1;
    LDS R25,0x010B      ; Load direct from data space
    EOR R24,R25     ; Exclusive OR
    STS 0x010B,R24      ; Store direct to data space

the proper way is also the most compact.

From experimentation in Atmel Studio's simulator, it seems that PIN= and PIN|= are equivalent, when the latter is optimized to SBI rather than STS, but I'm not sure if this can be relied upon (actually, it might also be that the compiler optimization to SBI should be illegal when writing to PIN). When PIN|= is compiled to STS, undesirable pin toggles happen indeed (in the simulator).

I think it would be safer to switch to using PIN= Alternatively, it might be worth considering a conditional optimization similar to this: https://github.com/Optiboot/optiboot/blob/81bdd404ff70b76fed716a10bbde90149d98e647/optiboot/bootloaders/optiboot/optiboot.c#L807-L811 To at least use PIN= when PIN|= would be compiled to STS

[WestfW]

The optimization of "PINx |= bit;" to "SBI" by the compiler, even though it's technically incorrect for some registers has been well-discussed in assorted fora. The truth is that C doesn't really have any concept of data whose written value is "unrelated" to the read value. I'm loathe to introduce conditional compilation or inline asm until this becomes an actual problem on a "popular" Optiboot configuration, though I agree that it's currently a bug for mega1280/2560/etc if you want to use a high-numbered port for the LED...

[stefano-zanotti-88]

OK. A brief comment explaining the (non-)issue would be useful though.

[WestfW]

Fixed. https://github.com/Optiboot/optiboot/commit/55d1e6b36922e4b8e3a32e6cea8ec03127ed65bf