Simplify PDI Wrapper Device Initialisation

[Remmirad]

Many of RIOTs peripheral driver interfaces use the pattern of a device type dev_t that is then initialized via init(dev_t). The current Rust wrapper around those interfaces expect a dev_t and construct a Rust type Device from it (see e.g. i2c-wrapper).

Someone inexperienced starting at those wrappers might now have a hard time of figuring out what those dev_t types are or rather how to choose them or what impact his choice has.

In C there exist macros like I2C_DEV(x) these macros take an index x and map this to an appropriate i2c_t. For example I2C_DEV(0) should give the "first" i2c_t of the board. These macros are not available in rust at the moment, but that can be easily changed in riot-sys. But this would still be just a binding in riot-sys.

My idea to embed this functionality in the rust-wrapper would be to use an rust API like this:

impl Device {
   fn new(dev_index) -> Device {
      dev = riot_sys::macro_DEV(dev_index);

This would also allow the wrapper to make the decision whether the obtained dev_t needs to be initialized or not. In RIOT all board provided dev_t types that can be initialized without additional parameters, are initialized in RIOT/drivers/periph_common/init.c. All other types are not. This again is a detail that might be hard to figure out for someone new to the matter. The wrappers could free the user of this decision if the assumption holds that: device types that need no further parameters are automatically initialized and those who do are not. This would simplify the use of the wrappers. If a device needs to be initialized then new would take the corresponding params and would do the initialization.

An additional unsafe method should still be provided which takes a raw dev_t if for some reason this is needed.

And in addition the documentation of the wrappers should point to RIOT/boards/<board>/include/periph_conf.h and RIOT/cpu/<cpu>/include/periph_cpu.h where hopefully details like static configuration of the devices are explained.

[chrysn]

The Hard Thing^TM about this is that the I2C_DEV macros have no fixed signature. If a platform decided that its devices are grid-numbered, it could require that its I2C devices be named I2C_DEV(4, 7), or even I2C_DEV("/dev/i2c4") -- the macros have no typing. I don't know of any devices on any platform that does this right now, but the choice of macros would seem to imply the option to do it.

If we could convince the RIOT maintainers that nailing this down to a single numeric (say, usize) parameter is a good thing, we could address this the way you suggest.

(There is the additional question of "which devices can we actually get as-exclusively-as-it-gets", but that's a harder question we don't have to solve at the same time).

[Remmirad]

Oh that is a problem in deed. I never thought about that. But it seems that they already treat it as if it was nailed down: like here? So maybe they could be convinced to e.g. write something in the docs or so. While at it the I2C_NUMOF macro would be very useful to have in the interface too.

[chrysn]

Good point, that might suffice to convince people that the ship for some board defining I2C_DEV(4, 7) style macros has sailed anyway :-)

[chrysn]

From current discussion on the RIOT channel, it seems that XXX_DEV is indeed fn(usize) -> xxx_t, and that furthermore the bounds are 0..XXX_NUMOF.

The logical next step would be to replace all xxx_t in the public APIs with an index as you suggested. That'd be an almost compatible change, but given that the xxx_t is often somethign like ufast8_t, it'd be a breaking change. Still better than to go through an API like new_from_index(), what do you think?

[Remmirad]

I think it would be nice to make the simple new the easiest and most intuitive one to use in the long term. One could decide to go with the new_from_ index() one first and save the change for when multiple breaking changes are accumulating and can be put together in one new breaking version that probably has to come at some point.

Hopefully in many cases people just wrote new(0, ...) and it should just continue to work?

[chrysn]

Even though there was only one small release in the 0.8 series, it has already accumulated some deprecations, so a 0.9 would switch the constructors to usize (as you said, non-breaking when used with literals) and drop everything that was deprecated, so most users can just update.

[Remmirad]

Is there already an Issue, PR about this "nailing down the macro" thing or should one be created to settle this thing in RIOT? Btw. do you have an idea how one would fixate the type of the macro? Just write something in the doc, or maybe make a function out of it? (The latter would of course require some work...)

[chrysn]

I think that the init code you found already does the nailing-down well enough; I'm starting on the macros for riot-sys.

Turns out some works was already started there (on GPIO_PIN); continuing that pattern.

[chrysn]

There is now https://github.com/RIOT-OS/rust-riot-sys/pull/17 that lays the groundwork. That one is not a breaking change yet, but would allow the pending PRs (UART and PWM) to already pioneer this PR's goal without doing any breaking changes.

[chrysn]

The way I figure the new interfaces should be is to have new(index: usize) -> Self functions that internally call riot_wrappers::macro_XXX_DEV(index), and that xxx_t would vanish from the public interfaces of the wrappers.

@Remmirad and @kbarning, could you give that a try in your PRs?