TWAI: Introduce the bitable crate for frame parsing.

[ocornu]

This is not meant to be merged!

Following up on our recent discussion, just to show what the code would look like:

• No more error-prone manual bit-mangling, with duplicates/derivatives here and there (check StandardId/ExtendedId parsing and writing e.g.)
• Cleaner definition and code
• Pure const design: disappears entirely at compile time (zero run-time cost)
• Added safety: reading/writing beyond a buffer's window fails at compile-time, with a descriptive message

It actually compiles into a smaller binary (admittedly, only a marginal 4 bytes). Runs fine on live CAN bus. HIL tests ok.

That is all! Thanks for your attention… 😁

[bugadani]

This obviously won't work. The crates.io page also doesn't link to a repo, which would be a nice to have.

[ocornu]

I know! I only pushed an early version to crates.io months ago as a crate name holder. I kept working on it as it's the bit-mangling lib the experimental PAC generating tool i'm playing with is based on. And i never took the time to freeze it, update crates.io and create the github repo…

I was under the impression, after our short talk, that it would not qualify for inclusion. But if you were considering it, i'd definitely get it done! 😁

Something that i did not mention (and i guess could be important): it does use several experimental compiler features to get its pure-const design to work… 😝

[bugadani]

experimental compiler features

good luck getting it to compile, then ;)

[ocornu]

I've never had a problem with the compiler i need to compile esp-hal. 😄

They're not totally unreasonable:

// Unstable compiler features required
#![feature(const_trait_impl)]
#![feature(const_convert)]
#![feature(const_option_ops)]
#![feature(const_type_name)]
#![feature(associated_type_defaults)]

Most of them, i believe, will get merged into stable at some point. Some of them are just there to get better error message (const_type_name) or just shorter/cleaner code (associated_type_defaults)…

[bugadani]

esp-hal needs to build with 1.95 stable, you can't use unstable features at all

[ocornu]

Well, that settles it then!

I'd have to check but i do not think this implementation would work at all without some of those… Which, to be honest, was in part intentional: there was no point redoing the runtime or macro-based approaches to bit-mangling libs that are already all over crates.io. By leveraging new features i could venture in much greener and less-traveled pastures.

[bugadani]

that's fine, happy to incorporate whenever it's all stabilised

[ocornu]

Great! My little pinky tells me this gonna leave me loads of time to polish it… 😅

Hey! While we're on the topic of external crates (and me blowing my own trumpet 😂): how about this one? It's tiny, straightforward, zero unstable features, zero runtime-cost.

It simply allows writing code that looked like this:

    pub fn new(raw: u16) -> Result<Self, EspTwaiError> {
        if raw <= 0x7FF {
            Ok(StandardId(raw))
        } else {
            Err(EspTwaiError::InvalidId)
        }
    }

…into code that looks like that:

    pub fn new(raw: u16) -> Result<Self, EspTwaiError> {
        (raw > 0x7FF).Err(EspTwaiError::InvalidId)?;
        Ok(StandardId(raw))
    }

Basically, chaining an error condition with Err()? (very "Rust-y" looking, imo).

It can't go wrong: it simply injects a trait implementation in core booleans (and, as its a non-snake-case method name, zero risk of collision with another custom trait for booleans).

I got used to using it, and all those if/then/else boilerplate guard clauses hurt my eyes now… 😅

Ok, i promise it was the last one! 😁

[bugadani]

I would rather use https://doc.rust-lang.org/std/primitive.bool.html#method.ok_or when it gets stabilized over weird dependencies allowing for non-idiomatic patterns. As far as I can see, this gets released in two weeks, as part of 1.98 - we'd need to raise MSRV, but we can probably do that after esp-hal 1.2

https://preview.redd.it/the-only-valid-measurement-of-code-quality-is-wtfs-minute-v0-btd5aebzr5h01.jpg?auto=webp&s=ab80b3546c7c5fd682350a952e6df1f83c9461e5

[ocornu]

Ah, nice! I wasn't aware of that one…

But no: even if it was stable i would rather avoid it:

• it introduces another "keyword" (ok_or()) when there's already a perfectly good and known one (Err())
• it's cryptic, at least for this use case: interpreting ok_or() as Err() is far from straightforward (how often are you gonna use the Ok(()) return value, realistically?)
• it focuses on the wrong condition: the "keep going" condition, when the relevant one for an early exit test is the error condition (its logical opposite)
• it's likely going to be slower, as in: on many hardware >MAX_VAL is faster than <=MAX_VAL

If it was indeed intended to do the same thing, it's a miss in my book. 😊

But to each their own!