Bus initialization procedure

[jnohlgard]

Some (most) of the drivers for i2c connected and SPI connected devices initialize the bus as a step in their own initialization procedure. I think this is bad practice, at least when more than one device is connected to the same bus.

In my opinion, it only makes sense to initialize the bus in the device driver for the bus slave if the slave device is the only device on the bus, but generally you can connect many devices to the bus and you (as application designer) need to decide on the greatest common subset of parameters which can be combined by looking over all of the bus slaves' parameters with regards to timing, polarity etc.

If this is not properly handled, some bus slaves may just not work at all (best case), or break, or it may bring down the entire bus, or seem fine on the development board, but the same application won't work on a customer's board (very difficult to debug).

I suggest that bus initialization (spi_init_master, i2c_init_master) should not be called from the bus slave initialization (e.g. mag3110_init etc.), and instead be called with a proper set of parameters for the application, before any bus slaves are initialized.

I don't think there is any non-convoluted way of automatically computing the best available parameters given a set of slave devices because of the great variation in different parameters, some devices have both minimum and maximum timings, some can work at either polarity, some can handle high baud rates but need a long idle time between each byte transferred etc. etc.. I think it is best if human application designers handle the bus parameters for the time being.

Opinions on this?

[jnohlgard]

@PeterKietzmann , @thomaseichinger , @jfischer-phytec-iot you have been active in device driver implementation lately, what are your thoughts on this?

[PeterKietzmann]

@gebart. I agree with you. Let me just list what came through my mind when thinking about this as a basis to discuss:

• we could stay with this solution and add peripheral configuration fields to the slave's init function to guaranty that all bus devices get the same parameters from an application (may blow up the slave's interface?)
• we could change to your proposed solution. Then we should introduce some kind of "spi initialized" flag or "spi_is_active" function to the peripheral driver so the slave device can check whether the peripheral bus is already initialized and otherwise throw an error
• don't know if I remember correct but did @haukepetersen had some arguments against a structure like that?

[PeterKietzmann]

In addition: When we decide to adapt the initialization procedure I assume this would be "a lot" of routine work. Could this be a Newbie Task?

[jnohlgard]

@PeterKietzmann The work will be quite routine, mostly deleting the init_master line from the driver and adding it back in tests/driver_xxx/main.c

When we reach a consensus on this and if the decision is to change the existing implementations I will create a tracker issue for all drivers which need modifying.

[PeterKietzmann]

Jupp. I'd like to hear at least one more opinion on that. This is a good newbie task

[thomaseichinger]

For the new network stack we will have to introduce an initialisation procedure which enables an application designer to granularly choose what to use. This could then be combined with initialising buses or other shared resources.

[PeterKietzmann]

Is there already a PR for that initialization procedure?

[jfischer-no]

@gebart @PeterKietzmann I think the idea is good.

we could change to your proposed solution. Then we should introduce some kind of "spi initialized" flag or "spi_is_active" function to the peripheral driver so the slave device can check whether the peripheral bus is already initialized and otherwise throw an error

And such a sensor-init function should querry the parameters of sensor drivers before bus initialization.
Did I understand that correctly?

[PeterKietzmann]

I didn't had this in mind. This could also be a solution but may introduce a bit more overhead then necessary. In my opinion it is up to the programmer to chose the correct parameters as this is a "natural" result of your thoughts. Like "I have this and that sensor on bus X -oh sensor A can just handle slow speeds- then I can not set the bus to high speed".
I proposed two solutions:

• Let the bus initialization inside the sensor-init function but forward the parameters from the application to guaranty there are no conflicts in bus-initialization
• Take the bus-initialization out of the sensor-drivers and do it just before sensor-initialization. In this case just check in the sensor-driver somehow if the bus is initialized (this is mainly @gebart proposal)

Now I tend to the second proposal because it's not needed to initialize the bus multiple times.

[jnohlgard]

@jfischer-phytec-iot That's an automatic solution, but that is what I meant with a convoluted solution because it is a pain to implement because of the diversity of different parameter requirements in different slave devices. It will also make the code size larger unless the optimization is performed offline (at compile time). Because of these arguments I prefer to let the application designer handle parameter definition and just use their values during bus initialization.

[jfischer-no]

Take the bus-initialization out of the sensor-drivers and do it just before sensor-initialization. In this case just check in the sensor-driver somehow if the bus is initialized (this is mainly @gebart proposal)

Ok.

[haukepetersen]

I put a little more thought into this and I think I might have a solution that I think could be feasible: how about we combine the best from both worlds:

Say we have a sensor driver that uses the SPI bus. In the device driver initialization we do then the following:

• Check if SPI bus is already initialized
  • If not: initialize the bus as we do currently
  • If yes: skip the initialization of the SPI bus

This way, for simple use cases (one device on the bus), you can just go on simply initializing your device and this will take care of everything regarding the bus. If you have a more sophisticated setup, you can initialize the bus manually before you bring up your device drivers - and thus the drivers will skip the bus initialization. The overhead is one simple bit compare and one if condition...

Regarding the peripheral API, we need to introduce 1 small function that keeps track about the peripheral init state, maybe something like

// spi.c:
static uint8_t spi_init_state = 0;

int spi_init(spi_t dev, ...) 
{
    ...
    spi_init_state |= (1 << dev);
    ...
}


// periph/spi.h:
extern uint8_t spi_init_state;

static inline int spi_is_init(spi_t dev) 
{
    return spi_init_state & (1 << dev);
}


// some device driver
int some_driver_init(..., spi_t spi, ...)
{
    ...
    if (!spi_is_init(spi)) {
        spi_init(spi, ...);
    }
    ...
}

[jnohlgard]

@haukepetersen I think it is better to use the strict initialization procedure.

Using the implicit initialization like you propose could potentially hide initialization mistakes because it just "happen to work" until the order of initialization of the device drivers changes.

[PeterKietzmann]

@haukepetersen this is approximately what I meant some days ago. Looking from the "only one sensor on a bus" side I do like your idea. But looking from the "multiple sensors on a bus" side I agree with @gebart that there might be (hidden) initialization conflicts. Imagine you forget (or event don't know) to initialize the bus before you initialize multiple sensors (this was the basic idea). Then the first sensor that is initialized will initialize the bus with its preferred parameters and we still have the problem of the smallest common parameters for all sensors on the bus. I would like to have a check in the sensor driver if the bus is initialized and otherwise return with an error. But the question is if the effort of an API change is reasonable for the benefit we get.
Is this reasonable or am I talking crap :-) ?

[haukepetersen]

@PeterKietzmann: Actually I think the complexity for building a system that check on compile or runtime for minimal common parameters is too big. Also I don't like this, as this leads to parameters not clearly being defined.

Imagine you forget (or event don't know) to initialize the bus before you initialize multiple sensors (this was the basic idea)

(i) this behavior is/needs to be clrearly stated in the documentation and (ii) I still have the strong opinion, that we can expect some extend of knowledge by the users/developers so that they are actually able to figure out the best parameter set for their setup and initialize their peripherals accordingly!

[PeterKietzmann]

Maybe I didn't stated my point that well.

• I am also against a system that checks parameters as this is too complex
• Regarding (i) is agree that this must be documented well
• Regarding (ii) I also agree that we can expect the knowledge of the best parameters by the users

I am pro for doing bus initializations outside the driver. I am sceptic about your proposed solution where the bus is initialized inside the sensor driver when the bus is not initialized, as this may cause conflicts again. I could imagine a solution where you just check in the sensor driver if the bus is initialized and if not return with an error. But this would also require an API change and may be discussed as the effort could exceed the benefit of this.

[jnohlgard]

@haukepetersen

(i) this behavior is/needs to be clrearly stated in the documentation and (ii) I still have the strong opinion, that we can expect some extend of knowledge by the users/developers so that they are actually able to figure out the best parameter set for their setup and initialize their peripherals accordingly!

I have the exact same opinion about the expectation in knowledge of the users, which is why I think we should let the users handle initializing.

@PeterKietzmann I agree with your arguments. Signalling an error when the bus has not been initialized before initializing the device driver would be helpful for developers, but requires an API change since many initialization functions return void IIRC.

[jnohlgard]

@haukepetersen What is your stance on this issue today?

I strongly believe in moving bus initialization outside the device drivers to allow for more precise control over bus parameters. I will run into problems in the future when I start using the on board memories on the Mulle together with the radio and accelerometer if I can not force the bus to go into the lowest common communication speed. (The memories can be driven at around 80 MHz or so)

I can perform the tedious work of moving the init calls outside the drivers on the code that is in-tree (drivers, tests/driver_*), but I can't modify applications for other people which may be outside of the open source project.

[PeterKietzmann]

I am still in favor of taking the initialization outside the driver. After reading our past comments I still think that the following could be a good solution, even if I'm not totally convinced that it is a "pretty" and efficient solution, but more feels like an alternative way of "getters" like e.g. in netdev.:
When taking the bus initialisation procedure outside a sensor driver, we could extend the bus driver APIs with a function that returns the speed a bus is initialised with. Of course an UNDEF parameter says that the bus isn't even initialised. A sensor driver checks this value and returns an error if the bus speed does not fit its requirements.

[haukepetersen]

Hej @gebart and @PeterKietzmann,
as you know, I am not at all convinced of taking the peripheral initialization out of the device drivers. My main reasons so far were (i) simplicity of usage, (ii) a driver knows best about it's needed parameters, and (iii) setups with contradicting parameters for a shared device were the absolute minority.

Now as we learned, (iii) does not hold anymore (and I guess we should have anticipated this...). (i) and (ii) do still hold. I think forcing a user to initialize the peripherals manually introduces quite some overhead/complexity for many use-cases. So as stated above, I would still opt for a dual solution, where you can (if you choose to) initialize your peripherals manually, but by default this is still done inside the device drivers.

[PeterKietzmann]

I think one could integrate this default configuration in my proposal above. Instead of just returning an error if the bus speed does not fit the requirements, one could also initialize the device with its default value. The outside initialization should have priority to leverage conflicts we want to solve within this issue :-).
Just to explain my general point of view: I also like to keep things as simple as possible and leave the detailed implementation concerns to the user. But as I can't acknowledge (iii) (maybe because of missing experience outside the OS world,) I just see @gebart 's problems which seem reasonable to me.

[lebrush]

Only for completion: It's also being tackled in #4926 for i2c. Current approach is to define a fixed speed for the bus in the periph_conf.h file.

[miri64]

So I guess #4780 and #4926 are fixing this issue?

[PeterKietzmann]

@miri64 as far as I'm currently involved I think yes. However, unfortunately both PRs were postponed for next release. @haukepetersen can you give a comment here and move the Milestone label then?

[miri64]

Please don't move the milestone but move the issue to the appropriate "known issues" column then.

[PeterKietzmann]

Only half of it will most likely be addressed within this release (#4780). Will move Milestone label.

[haukepetersen]

sounds good.

[smlng]

With the new I2C (and SPI) APIs in place this can be closed, right?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.