search

rp-rs / rp-hal-boards

Board Support Packages for RP2040 based PCBs
211 stars 87 forks source link

`rp2040-hal/rtic-monotonic` feature is not exposed #18

Closed Dblm0 closed 4 months ago

Dblm0 commented 1 year ago

Recently find out that rp2040-hal crate has its own rtic_monotonic::Monotonic trait implementation behind rtic-monotonic feature, but it's not exposed. Any reasons why?

9names commented 1 year ago

There's no reason, I think it was simply missed. If you wouldn't mind, could you make a PR adding it to all the boards?

jannic commented 1 year ago

What's missing, just a feature in the board crates which would activate the same feature in rp2040-hal? I wouldn't veto such a PR, but I wonder if it's the right approach to copy each feature of rp2040-hal to the board crates. Sounds like a lot of duplication, that doesn't scale well. And a firmware needing that feature could also activate the feature on rp2040-hal directly.

9names commented 1 year ago

What's missing, just a feature in the board crates which would activate the same feature in rp2040-hal?

Yep. We already re-export all the other HAL features, this is the only exception.

And a firmware needing that feature could also activate the feature on rp2040-hal directly.

They'd have to do their own import of rp2040-hal, you can't enable a feature that isn't re-exported. Doing so just to add one feature is not great.

jannic commented 1 year ago

Perhaps it was a mistake to just re-export the hal from the board crate?

In the end it's a question how things are explained: Currently, it looks like the right approach is to just depend on the board crate and use the re-export of the hal. That of course makes it very convenient, especially for short examples.

But one could as well argue that the firmware should actually depend on rp2040-hal, as that's what really used, and that the board crate is only a small addition, providing aliases for pins and details like that. And in practice, most firmwares will be large enough that the few lines of additional boilerplate don't matter much. I think the biggest disadvantage would be that when upgrading dependencies, one would have to select matching versions of HAL and board crates.

Dblm0 commented 1 year ago

I think the biggest disadvantage would be that when upgrading dependencies, one would have to select matching versions of HAL and board crates.

The only concern I have so far.

Finomnis commented 1 year ago

I think the biggest disadvantage would be that when upgrading dependencies, one would have to select matching versions of HAL and board crates.

The only concern I have so far.

I don't see how this is a concern as it will be compiler enforced. And changing the major version of a dependency is a major semver change anyway, it won't happen automatically/accidentally.

Dblm0 commented 4 months ago

Closing this, according to issue 69# comment

Finomnis commented 4 months ago

Be aware that the rtic-monotonic implemented in rp2040 is old, there was a complete rewrite of the rtic-monotonic trait a couple of months ago.