sys/auto_init: Fixed initialization of sht1x by maribu · Pull Request #10267 · RIOT-OS/RIOT

maribu · 2018-10-26T10:50:37Z

Contribution description

Commit cef81a1 introduced two bugs:

the driver for sht1x is initialized two times
the second initialization is done only when SAUL is used, but sht1x needs
to be initialized in any case. (SAUL registration is also done there, but
only when SAUL is actually being used.)

This commit fixes both.

Testing procedure

E.g. use examples/default on a board that features an SHT1x sensor, e.g. the MSB-A2. Prior to this PR RIOT will not boot due to the double initialization, with this PR applied RIOT will boot again.

maribu · 2018-10-26T10:51:54Z

@PeterKietzmann, @MichelRottleuthner: As you have an MSB-A2 board, does one of you mind to test this PR?

- the driver for sht1x is initialized two times - the second initialization is done only when SAUL is used, but sht1x needs to be initialized in any case. (SAUL registration is also done there, but only when SAUL is actually being used.) This commit fixes both.

maribu · 2018-10-28T07:35:13Z

After thinking about it I realized that the bug was introduced because adequate documentation of differences between the initialization of the sht1x and other sensors is missing. I will update this PR tomorrow to provide this documentation.

PeterKietzmann

Tested ACK

PeterKietzmann · 2018-10-29T08:30:21Z

@jia200x do you want this to be backported to the release branch?

jia200x · 2018-10-29T08:43:05Z

@PeterKietzmann yes, let's backport it ;). I will add the label

jia200x · 2018-10-29T08:44:55Z

@maribu could you provide a backport PR to 2018.10-branch?

The auto initialization of the sht1x module differs from the initialization of other sensors, but previously no documentation pointed that out. This lack of documentation led to a bug being introduced. This commit provides the previously missing documentation.

maribu · 2018-10-29T12:24:36Z

@jia200x: Sure. Both the added documentation and the bugfix, or only the bugfix? (I see little potential of breaking things by adding documentation, so I personally would go for both.)

PeterKietzmann · 2018-10-29T13:44:54Z

@maribu thanks for the documentation. To be honest, I don't understand what makes this driver so different from others. SAUL is one feature, but each driver should be auto-initialized if included via Makefile, no matter about SAUL.
@jia200x I didn't set the Reviewed: 5-documentation label. Can you please take over from here as I'm unavailable the next days?!

jia200x · 2018-10-29T13:49:40Z

@jia200x I didn't set the Reviewed: 5-documentation label. Can you please take over from here as I'm unavailable the next days?!

Sure, I take it-

@jia200x: Sure. Both the added documentation and the bugfix, or only the bugfix? (I see little potential of breaking things by adding documentation, so I personally would go for both.)

I think in this case it's not necessary. If the commits are the same and doesn't break master, then there are really low chances that breaks the Release. If something gets broken, it should be fixed in the master branch as well as the 2018.10-branch.

So, I would just backport this whole PR :)

maribu · 2018-10-29T14:38:22Z

To be honest, I don't understand what makes this driver so different from others. SAUL is one feature, but each driver should be auto-initialized if included via Makefile, no matter about SAUL.

I do agree with that. Providing known-to-work auto initialization will make life easier for application developers and using the foobar_params_t mechanism should still provide all the flexibility needed. However, all other sensor/actuator drivers will only perform auto initialization if SAUL is used as well, as they are wrapped into #ifdef MODULE_AUTO_INIT_SAUL ... #endif.

There is one drawback with the approach in the sht1x: The API is a bit clumsy. It asks for a pointer to an sht1x_dev_t, which is located in an global array in sht1x_auto_init.c.

This code snipped shows the clumsy-ness quite well:

RIOT/sys/shell/commands/sc_sht1x.c

Lines 32 to 56 in 289e635

    
           #define SHT1X_NUM     (sizeof(sht1x_params) / sizeof(sht1x_params[0])) 
        
           extern sht1x_dev_t sht1x_devs[SHT1X_NUM]; 
        
           static sht1x_dev_t *get_dev(int argc, char **argv) 
        
           { 
        
               switch (argc) { 
        
                   case 1: 
        
                       return &sht1x_devs[0]; 
        
                   case 2: 
        
                   { 
        
                       int pos = atoi(argv[1]); 
        
                       if ((pos < 0) || (pos >= (int)SHT1X_NUM)) { 
        
                           printf("No SHT10/SHT11/SHT15 device with number %i\n", pos); 
        
                           return NULL; 
        
                       } 
        
                       return &sht1x_devs[pos]; 
        
                   } 
        
                   default: 
        
                       break; 
        
               } 
        
               printf("Usage: %s [DEVICE_NUMBER]\n", argv[0]); 
        
               return NULL; 
        
           }

The shell command expects the user to give the number of the SHT1X device (e.g. 0 when only one is present) and has to get to the correct device "by hand". I was thinking about suggesting:

Use the same auto_init approach for every sensor/actuator as sht1x (that is: initialize the driver for each configured device regardless of SAUL, but only perform SAUL registration when SAUL is used.)

a) Change the API of all sensors and actuators so that the device descriptor is not passed as argument, but the array index of the device. (E.g. if two devices are configured via foobar_params_t devices 0 and 1 will be present.)

or

b) Extend the API with sht1x_dev_t sht1x_by_index(unsigned idx), so that the device descriptor can be obtained easily. (This is clearly less intrusive and more consistent to the rest of RIOT's APIs, but not as straight-forward as 2.a))

Once the cc110x rewrite is done I will create an PR that converts the sht1x API to suggestion b) to receive feedback on it.

maribu · 2018-10-29T17:07:36Z

@jia200x: The backport is done :-)

jia200x · 2018-11-02T09:54:53Z

ok, the documentation makes sense.
I would still try to make to make it compliant to other drivers in follow up PRs. We should maybe open an issue ;)

jia200x

ACK on my side

maribu force-pushed the autoinit_fix branch from 3bc364c to 6e1c50a Compare October 26, 2018 11:09

maribu mentioned this pull request Oct 26, 2018

drivers: Add driver for the LTC4150 coulomb counter #9653

Closed

PeterKietzmann self-assigned this Oct 29, 2018

PeterKietzmann approved these changes Oct 29, 2018

View reviewed changes

jia200x added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 29, 2018

maribu mentioned this pull request Oct 29, 2018

sys/auto_init: Fixed initialization of sht1x [backport 2018.10] #10289

Merged

jia200x added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Nov 2, 2018

jia200x approved these changes Nov 2, 2018

View reviewed changes

jia200x merged commit 6d8826e into RIOT-OS:master Nov 2, 2018

jia200x mentioned this pull request Nov 5, 2018

sys/auto_init: Fixed initialization of sht1x [backport 2018.10] #10326

Merged

maribu mentioned this pull request Dec 12, 2018

sys: single interrupt handler thread for interrupts in modules with blocking functions #10555

Merged

maribu deleted the autoinit_fix branch December 21, 2018 10:27

Conversation

maribu commented Oct 26, 2018

Contribution description

Testing procedure

Uh oh!

maribu commented Oct 26, 2018

Uh oh!

maribu commented Oct 28, 2018

Uh oh!

PeterKietzmann left a comment

Choose a reason for hiding this comment

Uh oh!

PeterKietzmann commented Oct 29, 2018

Uh oh!

jia200x commented Oct 29, 2018

Uh oh!

jia200x commented Oct 29, 2018

Uh oh!

maribu commented Oct 29, 2018

Uh oh!

PeterKietzmann commented Oct 29, 2018

Uh oh!

jia200x commented Oct 29, 2018

Uh oh!

maribu commented Oct 29, 2018

Uh oh!

maribu commented Oct 29, 2018

Uh oh!

jia200x commented Nov 2, 2018

Uh oh!

jia200x left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants