"uno-timer" example no longer works about avr-hal

[Rahix]

It seems the out_toggle implementation between static pins and dynamic pins differ:

Static Pin

avr-hal/avr-hal-generic/src/port.rs

Lines 723 to 725 in c1b78e9

	unsafe fn out_toggle(&mut self) {
	(*<$port>::ptr()).[<pin $name:lower>].write(|w| w.bits(1 << $pin))
	}

Dynamic Pin

avr-hal/avr-hal-generic/src/port.rs

Lines 645 to 651 in c1b78e9

	unsafe fn out_toggle(&mut self) {
	match self.port {
	$(DynamicPort::[<PORT $name>] => (*<$port>::ptr()).[<port $name:lower>].write(|w| {
	w.bits(self.mask)
	}),)+
	}
	}

The dynamic pin impl is writing to the PORT register instead of the PIN register. This seems to be a regression from commit 063f845. Paging @LuigiPiucco :)

@gregoryholder, can you change [<port $name:lower>] to [<pin $name:lower>] in L647 and test whether that fixes the problem?

[LuigiPiucco]

This seems a bit strange, at least upon first glance. The dynamic version is not working, but (in theory) the one that's wrong is the static one (out methods should write to port, not pin, since pin is for reading when input, no?). Also, I notice some of these use .write and others .modify. I didn't pay much mind to it before (just copy-pasted) but maybe it's similar to #494, where the former causes unintended consequences. I'll look into it and take this opportunity to apply a usage comment like the one for the ADC. Just note I don't have an Uno right now, so I'll try to reproduce on the Mega2560. If I can't I'll ask @gregoryholder to check the fix when I finish.

[gregoryholder]

I can confirm this happenes on a mega2560, that's what I was using initially. To sanity check I cloned the latest version of the repo and ran the unchanged example on an Uno.

…

On Fri, Jan 26, 2024, 18:32 Luigi Sartor Piucco ***@***.***> wrote: This seems a bit strange, at least upon first glance. The dynamic version is not working, but (in theory) the one that's wrong is the static one (out methods should write to port, not pin, since pin is for reading when input, no?). Also, I notice some of these use .write and others .modify. I didn't pay much mind to it before (just copy-pasted) but maybe it's similar to #494 <#494>, where the former causes unintended consequences. I'll look into it and take this opportunity to apply a usage comment like the one for the ADC. Just note I don't have an Uno right now, so I'll try to reproduce on the Mega2560. If I can't I'll ask @gregoryholder <https://github.com/gregoryholder> to check the fix when I finish. — Reply to this email directly, view it on GitHub <#499 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BFROZQOVGN5P6C6DOEWZJ23YQPSEBAVCNFSM6AAAAABCLE7KBKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJSGQZDMNBXGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[LuigiPiucco]

I'm reasonably sure on the solution (here), but I can't test right now, so I'll wait before opening a PR.

Reading through #284 I am a bit confused, and I think the source of this bug was similar confusion about the behavior of registers in Rust. The code of out_toggle before the mentioned issue was:

$<port pointer wrapper>.modify(|r, w| {
    w.bits(r.bits() | self.mask) // (1 << $pin_num) as the mask for the static version
})

... which, looking closely, is what out_set expands to now. And doing some truth table analysis, this can only ever set the bit, but never clear it. For toggle, we want something that does both, kind of like this:

match cur_bit {
    0b1 => 0b0, // Clear
    0b0 => 0b1, // Set
}

The proposed solution was:

$<port pointer wrapper>.write(|w| w.bits(self.mask))

... which could work, if the behavior of .write was to flip from the port every bit where the mask is 1. But I'm pretty sure it just writes whatever you pass unchanged, including the 0s. The only exception I know is if you use single field access instead of a bitmask, when it assumes the reset value for anything you don't touch. So we do need .modify, at least to keep other bits unmodified.

Another issue is what function to use between the current value and the mask to toggle the one bit, keeping others. The answer is an XOR. I think in this case, the notation of C makes thinks a bit more evident:

PORTD |= 1 << 3;    // Set bit 3
PORTD &= ~(1 << 3); // Clear bit 3
PORTD ^= 1 << 3;    // Toggle bit 3, for every 0 in the mask it does not change PORTD, but for the 1 it flips

So we end up with:

$<port pointer wrapper>.modify(|r, w| {
    w.bits(r.bits() ^ self.mask)
})

Please double check the logic here, it does seem easy to get wrong. There's also some analysis of assembly in #284, but due to the reasons above I think it doesn't apply. I can check the output of my suggestion if needed, though.

[Rahix]

@LuigiPiucco, writing a single 1 bit into the PIN register toggles the pin:

That's why I was suggesting that the problem is writing to PORT instead of PIN. :)

[LuigiPiucco]

Ok, I did not know that. That's an interesting piece of information. However, unlike in #284, the compiler never produces sbi for me (not even in the static case). It's always ldi-out. The XOR code as I suggested compiles down to in-modify-out on the PORTx register. Without this toggle feature on the PINx register, I think that's as good as we'd get. But since it exists, we can rule out the in and eor. Do you prefer to change to that or keep the more recognizable XOR approach? Also, I think the static case may be possible to optimize with assembly (guaranteeing it translates to sbi), but not the dynamic one, since sbi encodes the bit and register into the opcode.

[Rahix]

We should use the "write bit to PIN reg to toggle" trick. Two reasons:

1. It's faster than a read-modify-write cycle
2. It's atomic. The read-modify-write cycle of writing to PORT would require to be put in a critical section to avoid a nasty race condition. This would be even more overhead.

---

However, unlike in #284, the compiler never produces sbi for me (not even in the static case). It's always ldi-out.

I assume you were testing the current implementation? That one indeed cannot be optimized to sbi anymore. sbi is emitted for a static version of the read-modify-write cycle. But we didn't want to rely on this optimization for correctness, so we decided to move to the current approach which leads to ldi-out. It matters for correctness for the same race-condition mentioned above.

[LuigiPiucco]

@Rahix I updated my branch and I am writing the pull request with details. I deviated from just changing port to pin, so I'll comment here if you want to start checking it before I write the detailed explanation. I wrote it in a way that the optimal single sbi/cbi is always generated for setting and toggling, even in lower optimization levels. It's also "safer" (in the sense that it uses less unsafe {}s). The getters generate more code than the setters, but I think comparing to the prior implementation there's negative balance. But maybe there's something I'm missing, please check carefully. I also only looked at the bytecode, not real execution.