tact1m4n3 / crsf-rs

A work in progress crsf protocol packet parser
MIT License
10 stars 7 forks source link

Use static CRC8 table #17

Closed peterkrull closed 4 months ago

peterkrull commented 4 months ago

Solves #16.

Here I have not added #[link_section = ".data"] which could increase performance on on embedded devices further, since it hints to the linker that the table should live in RAM rather than flash. The reason is, I am not sure if there could be side effects from just having it there for everyone. We could make it a feature:

#[cfg_attr(feature = "ramcrc", link_section = ".data")]
static CRC8_ALG: Crc<u8> = Crc::<u8>::new(&CRC_8_DVB_S2);

but maybe that is too subtle of a real-world difference to justify its own feature flag 😅

anti-social commented 4 months ago

I'm not an expert in embedded development but I wonder how static atomics work if they are placed in flash memory? I use them a lot and of course I modify them, so they should be placed in RAM.

Also rust says that:

the program's behavior with overridden link sections on items is unpredictable and Rust cannot provide guarantees when you manually override them

peterkrull commented 4 months ago

As far as I understand, static values which under no circumstance can be modified, may be placed in read-only flash. So that means no static mut, no interior mutability, no atomics. I am also no expert on this though. I would be just as happy to leave it static as-is without the linker hack, since it is already way faster than how it is currently.

tact1m4n3 commented 4 months ago

It's a static non-mutable variable so I think there's no problem in placing it in ram. As far as I know, the only difference between flash and ram is that in ram it would have to be copied at runtime, but it's not that large anyway. In the future, we can probably make an owned version in the PacketReader/Parser struct if we move the rest of the parsing there(at least the checksum validation). A raw packet is currently not validated. But this would raise the problem that the dump packet function would not have access to the crc table anymore. So a static version might be the best approach.

tact1m4n3 commented 4 months ago

As @peterkrull suggested, we could validate the packet as we are reading it.