RFC: Investigate the need for a lower-level layer of abstraction

[jessebraham]

(This is a little all over the place right now, just trying to get my thoughts on this written down; will edit this as my thoughts clarify)

Some low-level implementation details are fundamentally required, as not everything can be abstracted over cleanly. You see most of this taking place in the soc module, with per-chip implementations for various functionality. While this is a necessity, in its current state there is a lot of duplication still happening, and I think there's a lot of room for improvement.

One big win would be eliminating as much of the raw register accesses as possible, and instead updating our PACs to include the relevant registers/fields. This is currently being tracked in its own issue:

• Remove usages of raw register manipulation outside of PAC crates #1194

This may or may not be possible/practical for all registers/fields, however I'm sure we can make some great improvements.

Beyond this, some raw register access may still be required. In this case, we should at least make efforts to de-duplicate the helper functions/macros being used. For instance, reg_set_bit is defined in each of the soc::$chip::trng modules as well as in each of the clock::clocks_ll::$chip modules. There are likely other such instances lurking as well.

Maybe there is a better way to do this than consts/macros too, I'm not sure.

With all this said, it's not clear to me whether or not we should try to form some low-level HAL API in addition to what we're doing now. This is done in ESP-IDF, for instance, however I suppose it's worth some discussion and reflection how to best handle this all moving forward as complexity continues to grow. There's no immediate need for action, just something to consider for now.

[bjoernQ]

One big win would be eliminating as much of the raw register accesses as possible, and instead updating our PACs to include the relevant registers/fields.

I'd love that. I think the reason why those things currently look how they look is that they the code is ported straight away from esp-idf and initially it made sense to make it look as closely to what the C code looks like to enable us to compare it to the C code easily ... getting those things to work is amazingly annoying and going multiple steps at once doesn't help. But clearly the next step is to make that code actually look like Rust - not C in Rust syntax 😄

On the other hand, there was a reason to not do it immediately. As long as we are not sure what our code does is 100% correct it's good to have it more or less easily comparable to esp-idf. Especially since many of the things there are not documented well or at all in the TRMs

But most probably our code there is proven enough

...we should at least make efforts to de-duplicate the helper functions/macros being used...

Totally agree.

With all this said, it's not clear to me whether or not we should try to form some low-level HAL API in addition to what we're doing now.

I guess this refers to all drivers in general, not just the SOC module? And I guess it's also referring to esp-idf's *_ll.h?

I think we already have something like that in many drivers (the Instance construct). I think I like that.

Having something like a ll module is something I thought would be a good idea in the past but currently I have doubts. I guess it will make it harder to understand what is happening - at least when reading esp-idf code the multiple levels of abstraction make it a bit harder to follow in case I want to truly understand what is happening in detail (i.e. down to register level) especially if I'm looking for differences for different chips. There is also a comment in the same direction here: #1232 (review)

I'm not saying we shouldn't do something like that but I think, it won't be an "makes everything better" thing and we should carefully balance it

[MabezDev]

IMO the pac is the low-level layer. The SoC module does have some overlap, but to me it's just a convenient place to put chip-specific stuff. I think most of our need for a ll layer would be solved by:

1. Closing Remove usages of raw register manipulation outside of PAC crates #1194
2. Investing more time in the PACs, (I'm am very guilty of not doing this 😅), but @burrbull's recent contributions, in particular use DMA channels #1330 has really shown how much we can clean up our drivers by ensuring our PACs are in a good spot