Fix toggle() for some old microcontrollers

[andber1]

Some microcontrollers do not support toggling the output via the PIN register. On these microcontrollers the PIN register is readonly and toggling is done by reading and modifying the PORT register.

Microcontroller	atomic toggle supported?
atmega48p	✅
atmega16	❌
atmega164pa	✅
atmega168	✅
atmega328p	✅
atmega328pb	✅
atmega32a	❌
atmega32u4	✅
atmega2560	✅
atmega128a	❌
atmega1280	✅
atmega1284p	✅
atmega8	❌
atmega88p	✅
attiny2313	✅
attiny167	✅
attiny84	✅
attiny85	✅
attiny88	✅

I tested this on an ATMega16.

Related to #501.

[Rahix]

As I had mentioned in #501 (comment), the current code is correct for most AVR MCUs. But it seems ATmega16 does not support this?

I think we'll have to special-case the toggle code in some way for MCUs that don't support atomic pin toggling then...

Unfortunately, it gets even more complex because the code you suggested is not atomically toggling the pin either. So for ATmega16, the register access would additionally need to be wrapped in a critical section.

[andber1]

Yes, ATmega16 does not support toggling the output with the PIN register. The PIN register is readonly (see here, p.66). At least ATmega8 (datasheet, p.65) and ATmega32A (datasheet, p.70) do not support this feature either. Maybe there are some more MCUs that do not support his. I did not read all datasheets.

What is your idea for handling the special cases? A new parameter for the impl_port_traditional macro?

Can you explain the atomic issue in more detail? As far as I know I cannot have multiple pin references at the same time? So it shouldn't be a problem, right? Am I missing something? In addition there are other occurrences of modify(|r, w|... in ports.rs, that are not atomic, too.

[Rahix]

A new parameter for the impl_port_traditional macro?

Yes, something like this. Whatever way requires the least macro trickery to jump through.

One idea that comes to mind: A boolean literal macro variable which is then checked in a static if condition. This gets optimized away and should be doable without too much macro vodoo.

#[inline]
unsafe fn out_toggle(&mut self) {
    // +-- this macro variable needs to be defined somehwere in the macro
    // |   pattern as an `:literal` and then filled with `true` or `false`
    // |   during macro calls.
    // v
    if $chip_supports_atomic_toggle {
        (*<$port>::ptr()).[<pin $name:lower>].write(|w| {
            w.[<p $name:lower $pin>]().set_bit()
        })
    } else {
        // TODO: alternative impl goes here
    }
}

Can you explain the atomic issue in more detail?

The problems are present when two tasks try to access two pins in the same bank (=same registers) concurrently. Mostly, this comes up with interactions with interrupt routines.

All pin operations must be written in a way where concurrent accesses cannot lead to inconsistent state. There are two ways to do this:

1. Perform the access in a single instruction.
2. Perform the access inside of a critical section.

Right now, most of the pin operations leverage the first option.

Am I missing something? In addition there are other occurrences of modify(|r, w|... in ports.rs, that are not atomic, too.

This is where things get really tricky: The compiler is able to optimize some modify() accesses down into a single instruction which is then an atomic access. This works for modify() operations where only a single bit with constant offset is set or cleared. Such operations are lowered into sbi/cbi instructions.

The reason your modify() is not optimizable is that whether the bit is set or cleared depends on previous state. So the compiler will leave the non-atomic read-modify-write sequence in place.

You may be able to use the sbi/cbi optimization by writing the toggle() as

if self.out_get() {
  self.out_clear();
} else {
  self.out_set();
}

but you need to verify that this actually lowers to atomic register accesses and it should be evaluated whether this leads to less instructions than just wrapping your modify() in a critical section.

If you have questions, let me know.

[andber1]

Thanks very much for your detailed explanations. That is very helpful. I will try to implement your suggestions and update this pull request soon.

[andber1]

but you need to verify that this actually lowers to atomic register accesses and it should be evaluated whether this leads to less instructions than just wrapping your modify() in a critical section.

The if else solution needs one more instruction than the critical section solution. So I used the critical section here.

[Rahix]

On these microcontrollers the PIN register is readonly and toggling is done by reading and modifying the PORT register.

I think this is unfortunately not a reliable indicator. To give two examples, here is an excerpt from the ATmega328P datasheet, stating that it does support the feature:

And this is from the ATmega168 datasheet:

I think the only way to figure this out is to check each datasheet for a similar passage :( I would assume that most chips do support this though, only that one very old family seems not to.

[andber1]

You are right. The following microcontrollers do indeed support this feature: atmega48p, atmega88p, atmega168, atmega328p, attiny88
I have updated the code and the table above.

[Rahix]

Very nice, thanks a lot for fixing this!