generic: improve bit operations usage

[LuigiPiucco]

Reading the discussion in the fixed #499 is important to understand this. For the dynamic case, I ended up just changing the typo, since its dynamic nature prevents optimizing further without some real arcane magic. But I also changed the static one to use fields, which look more sound and always generate the optimized bytecode.

All low-level operations with registers in PinOps were implemented with raw bit-manipulation, a là C. The method used from the readers/writers of avr-device was .bits(), which takes/gets a full u8 bitmask, to which changes were made via bitwise operators. This is the most "expected" approach, but digging through the PAC documentation, Rust's static typing and discrimination of ports and pins via types allows us to write code that really reads off as "write to this single bit in the register" as opposed to "write this 1-bit-changed mask to the whole register". Moreover, when compiled down, AVR actually has instructions for single bit operations on IO registers, so Rust turns it into just (s|c)bi $IO,$bit, where $IO and $bit are static/immediate.

Let's look at an example expansion of the generator macro for a known-type Pin:

unsafe fn out_set(&mut self) {
    (*<PORTD>::ptr()).portd.modify(|_, w| {
        w.pd2().set_bit()
    })
}

Because 1) we used fields instead of the .bits method, 2) we only touched 1 bit and 3) we used .modify, therefore saying "only change what I explicitly request", the compiler knows this can be safely turned into sbi 0x2B,0x2. out_clear is the same, the instruction just changes to cbi. Toggle would need to be an XOR, but due to the feature that writing 1 to pind toggles the corresponding bit in portd, it can be exactly like above, replacing portd with pind as the self for .modify. To make it consistent, I changed the read implementation to also use these field accessors. In the known-pin case, it's:

unsafe fn out_get(&mut self) -> bool {
    (*<PORTD>::ptr()).portd.read().pd2().bit() // Returns bool, no need to use != 0.
}

This generates a read with in r24,0x2B, then some manipulation to make the value either 0b00000000 or 0b00000001 as Rust mandates for bool. I tested two different pins, the manipulations it generates depend on the position of the 1 in the byte, so maybe there's some room for improvement here.

For the unknown-pin case, I tried many approaches, but short of somehow storing the static methods and deferring to those (which I couldn't implement), the .bits-style beats all of them. So I kept it there, with some minor reformatting.

[Rahix]

Nice, seems your changeset is already uncovering a bunch of pins that the HAL defines but which don't actually exist. I didn't check all the CI failures but at least PC7 on ATmega328PB and PG0 on ATmega128A are erroneously defined. Can you add commits to this PR fixing the failing chips?

[LuigiPiucco]

I'm already preparing one for the Atmegas, but there's an issue with the Attinys: They define no fields. Can you check why that is? Maybe some issue with the svds?

[Rahix]

I like the changes here (especially as they are seemingly already uncovering real bugs) but I think this is orthogonal to the #499 bug. I have created a separate PR which just fixes #499 so the two topics don't get mixed together too much (#501).

[Rahix]

They define no fields. Can you check why that is? Maybe some issue with the svds?

The pin fields are generated from the ATDF (https://github.com/Rahix/atdf2svd/blob/main/src/atdf/patch.rs#L11-L56) so we're relying on correct info in the ATDF. If that's missing (and we can't find the info elsewhere in the ATDF file), I guess we have to manually patch the SVDs so they include the correct pin info...

[LuigiPiucco]

I like the changes here (especially as they are seemingly already uncovering real bugs) but I think this is orthogonal to the #499 bug. I have created a separate PR which just fixes #499 so the two topics don't get mixed together too much (#501).

Okay, that's indeed better. I'll remove the change from here to avoid conflict.

[Rahix]

@LuigiPiucco is it just the ATtiny2312{,A} or are other MCUs also missing the pins?

[Rahix]

(Taking care of the ATtiny2313 pins now)

[LuigiPiucco]

I've checked attiny2313, attiny167 and attiny84, none have them. I compared the ATDFs for the attiny2313 and the atmega2560, it seems the difference is a signals tag beside the register-group in each instance.

[Rahix]

Alright, I'll take care of the avr-device patches.

[LuigiPiucco]

When were the atdfs last updated? The commit history for the attiny2313 says 3 years, the packs repo's latest version is 2.0.368 (2022-03-02). Maybe this has been fixed upstream?

[Rahix]

If you want to, you can check the new release but I doubt that these chips will have received any updates in the ATDF files. In any case, I've hopefully taken care of the pin fields manually now (Rahix/avr-device#148, Rahix/avr-device#149)...

[Rahix]

If you want to, you can temporarily add this change to your branch to make the CI attempt every build:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index e6829e98bf..06f1b37492 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -12,7 +12,7 @@ jobs:
   ci:
     name: "${{ matrix.m.type }}: ${{ matrix.m.name }}"
     strategy:
-      fail-fast: true
+      fail-fast: false
       matrix:
         m:
           - type: board

Then it'll be easier to see which MCUs are still failing...

[LuigiPiucco]

It seems the atmega32u4 atdf is also incorrect, it says port F has pins 0..6, but the datasheet and the Leonardo pinout say 0..1 + 4..7.

This is why arduino-leonardo and sparkfun-promicro are still failing.

[Rahix]

Interesting, will fix as well. Do the tiny series chips build against my updated avr-device now?

[LuigiPiucco]

I tried adding it as a git dependency, bit it fails due to a missing generic.rs in lib.rs. I think it's related to Rahix/avr-device@dbbbc6e.

[Rahix]

I ran local build-tests against avr-device for the failing platforms, seems to work now. Published a new avr-device version: https://github.com/Rahix/avr-device/releases/tag/v0.5.4 And it's already pulled into avr-hal: #502

Please rebase on latest main :)

[Rahix]

I tried adding it as a git dependency

For future reference: git-dependency unfortunately does not work because the codegen steps of avr-device don't run when you do that. so it'll always throw errors about missing files.

The only thing you can do is patch in a local path dependency to a checkout of avr-device where you ran the codegen steps (i.e. make):

# Cargo.toml
[patch.crates-io]
avr-device = { path = "../avr-device" }

[LuigiPiucco]

Everything seems to build now, but I forgot to change fail-fast to true again, just edit that out later. If I do it it's going to run CI needlessly.

[Rahix]

Everything seems to build now, but I forgot to change fail-fast to true again, just edit that out later. If I do it it's going to run CI needlessly.

Ah, don't worry about the superfluous CI run. I'd prefer to drop the change from the commit here so it doesn't show up in the diff later on.

[Rahix]

Thanks! Again, I love how this rather simple change managed to uncover quite a few bugs in one go.

However, I still have one comment related to the out_toggle() story, see below.

[LuigiPiucco]

I think this is ready. I made the change from .modify to .write, but read the comment on the resolved thread, it contains a lead as to allowing the compiler to better optimize, and also enhance the avr-device API. I'll PR the results later if they are satisfactory.