Skip to content

sys/auto_init: Allow to auto-initalize sensors and actuators independ of SAUL#11871

Closed
maribu wants to merge 1 commit intoRIOT-OS:masterfrom
maribu:auto_init_sensors_actuators
Closed

sys/auto_init: Allow to auto-initalize sensors and actuators independ of SAUL#11871
maribu wants to merge 1 commit intoRIOT-OS:masterfrom
maribu:auto_init_sensors_actuators

Conversation

@maribu
Copy link
Member

@maribu maribu commented Jul 19, 2019

Contribution description

  • Added module auto_init_actuators_sensors to auto-initialize actuators and sensors
    • auto-initialization for sensor driver foo and actuator driver bar will be implemented in submodule auto_init_actuators_sensors_foo and auto_init_actuators_sensors_bar, respectively
  • Pseudo-module auto_init_actuators_sensors_default is intended to pull in sensor and actuator auto_init submodules for all drivers used
  • Made auto_init_saul depend on them on auto_init_actuators_sensors_default

Testing procedure

This cannot be reasonable be done without a user of the new interface. Therefore, please head over to #11872 to test the adaption of the SHT1X driver to this new auto_init concept, once the fundamentals and the API proposed here are agreed upon.

Issues/PRs references

Implements the seemingly reached consensus in Issue #11826

@maribu maribu added Area: SAUL Area: Sensor/Actuator Uber Layer Area: sys Area: System Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Jul 19, 2019
@maribu maribu changed the title sys/auto_init: Add modules for sensors & actuators sys/auto_init: Allow to auto-initalize sensors and actuators independ of SAUL Jul 19, 2019
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks way better than I imagined, well done.
But if I understand correctly, there is no way to de/activate auto_init for specific drivers only? If so, is this wanted?

Also I would appreciate more verbose documentation - but it's obvsly not blocking.

I don't really want to ack/block this at all since I don't see the need for it and don't agree with all the arguments. I prefer another maintainer (@kaspar030?) takes the responsibility. Summon me if this PR get's stuck.

@maribu
Copy link
Member Author

maribu commented Jul 20, 2019

Also I would appreciate more verbose documentation

I'll add that on Monday (hopefully, otherwise as soon as I can).

@maribu
Copy link
Member Author

maribu commented Jul 22, 2019

@SemjonKerner: I extended the documentation as you suggested.

@maribu
Copy link
Member Author

maribu commented Aug 21, 2019

OK, no one seems to complain :-) @MichelRottleuthner: Mind to take a look?

(Btw: It is good that the merge conflict is showing. This PR should never have touched that file. I'll rebase to fix that.)

@maribu maribu force-pushed the auto_init_sensors_actuators branch from c1d022c to 27d17d8 Compare August 21, 2019 07:11
@aabadie
Copy link
Contributor

aabadie commented Aug 21, 2019

Why not just have one auto_init_drivers module instead of having one for sensors and one for actuators, which are basically the same ?

@maribu
Copy link
Member Author

maribu commented Aug 21, 2019

Why not just have one auto_init_drivers module instead of having one for sensors and one for actuators, which are basically the same ?

I have nothing against merging them. (There are some reasons to keep them separated though, for one both groups are continuously growing and having them separate roughly cuts the number of drivers in the group in half; two smaller lists are easier to read than one long list. Secondly, someone wanting all sensors auto-initalized but manually initializing the actuators will find this separation useful.)

If they should be merged in to a single group, I think just "drivers" would not be the ideal name, as e.g. network device drivers are already in the netif group. In that case I'd say auto_init_actuators_sensors or auto_init_sensors_actuators would be a better name for the merged group.

@maribu
Copy link
Member Author

maribu commented Oct 17, 2019

Ping! (Also: please give feedback on whether I should merge auto init for sensors and actuators to the same auto_init submodule. To me, either is fine.)

@ghost
Copy link

ghost commented Oct 17, 2019

Hey, merging both makes sense to me. I can't think of a use case where it is important to differentiate that way. Also it resembles the file structure of the code base and it will look cleaner and minimize new keywords.

Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general idea to have some user-friendly, simple was to initialize devices without a dependency to SAUL is quite nice, thanks for taking this on!

With the current state of the code/concept I see at least two problems:

  • IMHO it does not make sense to split between sensors and actuators. Why even focus on these two - the approach is valid for all supported devices!
  • the current code leads to significant code duplication comparing it to all those ugly saul/auto_init_xx.c files. So we should actually combine both: use the generic auto_init for devices, an put the SAUL auto init on top of it

In terms or requirements, I think we should keep the following in mind:

  • all the auto_init should be optional, it must always be possible to manually configure/initialize device from a use application for complete control
  • the configuration (submodule-names and effects) should be as intuitive as possible so its always clear which devices are actually auto-initialized and which are not
  • we should make sure to integrate nicely with existing auto-init blocks -> how about combining this also with auto_init/netif?!

@maribu
Copy link
Member Author

maribu commented Oct 18, 2019

* the approach is valid for all supported devices!

+1

But lets focus on sensors and actuators for now. First, not everyone seems to agree on that (while everyone no at least can tolerate the auto init for sensors and actuators). Second, it makes sense to me to do this is smaller steps. (To me: One PR for the infrastructure of each driver group and one PR per driver. This should PR should the infrastructure for auto initialization of sensors and actuators.)

* So we should actually combine both: use the generic auto_init for devices, an put the SAUL auto init on top of it

+1

This is actually exactly what I proposed and what I did in the proof of concept implementation. The auto_init_saul will eventually be a pseudo-module that unlocks the saul registration in the already existing auto init code of sensors and actuators. But I'd like to have them coexisting for now, so that the transition can be done one module at a time.

* all the auto_init should be optional, it must always be possible to manually configure/initialize device from a use application for complete control

+1

This was the single point in the discussion so far everyone agreed upon, and everyone did so strongly as far as I can tell.

* the configuration (submodule-names and effects) should be as intuitive as possible so its always clear which devices are actually auto-initialized and which are not

+1

Maybe auto_init_<MODULE_NAME> would be more intuitive for users that only one to enable auto init for specific drivers than auto_init_<GROUP>_<MODULE_NAME> (as suggested here). But on the other hand someone wanting to add modules to auto init by group could be surprised which modules are added.

Maybe the pseudomodule to auto-init everything (that can be sensible auto-initialized) is best named auto_init_all - the should be no confusion on what that pulls in.

I'm also not sure if there is actually a use case of wanting to add modules to auto init by group. I can easily see someone generally wanting to auto init the modules, with the exception of one or two modules. That would be best done by USEMODULE += auto_init_all and DISABLE_MODULE += auto_init_foo to have all but foo autoinitialized. And I also can easily see someone generally not wainting to autoinitialize modules, with the exception of one or two modules. But I can not see a use case where one wants to have a large number of modules autoinitialized and another large number not autoinitialized. So maybe the whole add modules to auto init by group feature is not needed at all?

* we should make sure to integrate nicely with existing auto-init blocks -> how about combining this also with `auto_init/netif`?!

Can you elaborate on that? So for the auto init implementations seem to be deliberately independent of each other, so that the do not have and inter-dependencies or interactions. With the huge number of individual auto_init implementations, I think it would easily become a maintenance mess if would no longer be able to treat same is completely isolated from each other. The number of possible auto_init configurations with n auto init implementations is:

\sum_{i=0}^n \binom{n}{i}

That is too huge to test. So we really should have them as isolated as possible IMHO.

@fjmolinas
Copy link
Contributor

@maribu for keeping all the autoinit stuff optional maybe we can adapt this in the same direction as #13089, so basically setting them as DEFAULT_MODULE? I'm interested in getting the ball rolling again on this PR and I alike the approach you took here.

@maribu
Copy link
Member Author

maribu commented Jan 22, 2020

maybe we can adapt this in the same direction as #13089

I like the idea. I hope I get time to push this forward again; maybe during my parental leave that is expected to start in a few weeks.

@maribu maribu force-pushed the auto_init_sensors_actuators branch 2 times, most recently from 04e456d to 66c4dd2 Compare February 9, 2020 21:30
@maribu
Copy link
Member Author

maribu commented Feb 9, 2020

I updated this PR to merge the auto initialization of sensors and actuators and solved the merge conflicts. The proof of concept implementation in #11872 has been updated on top of this.

@maribu
Copy link
Member Author

maribu commented Oct 14, 2020

I have exact that problem, mentioned in #11871 (comment). I need the well defined stuff from auto_init(), but I need to call auto_init() at a dedicated place in main(), because of power management.

If the auto_init module is added to DISABLE_MODULE, it filters out all auto_init_* modules and pseudomodules, and I a lot of error messages, if I not add auto_init_% to DISABLE_MODULE, but this disables all the auto-init stuff, that I need.

Hmm, as the issue is with the upstream RIOT, I'd say it is better to discuss this in an issue. You can reference this PR, if this PR should take lessons learned into account.

Could you also provide some details? Maybe you could share a trimmed down version of your code? (Only the initialization part, the actual application logic is most likely not relevant for the issue.) That way it would be easier to understand the issue. Feel free to mention me in the issue, so that I have a look.

@fjmolinas
Copy link
Contributor

+1 for going forward with this one. I do think it might make sense to make a distinction between the init function and the automatic initialization of that driver/sensor/actuator, kind of what is going on for netif where there is gnrc_netif_init_devs and auto_init_gnrc_netif. But lets discuss this somewhere else and not delay any further.

What about EEPROMs, displays or voltage regulators?

This is why an auto_init_drivers would fit better IMHO (and I already proposed that ;))

I would also say that fine grained is better, but maybe not always justified, I would think it would be best to have:

  • auto_init_drivers
    • auto_init_sensors
    • auto_init_actuators (or auto_init_sensors_actuators)
    • all other drivers where having a distinct category is not yet worth it

@ghost
Copy link

ghost commented Oct 21, 2020

I would also say that fine grained is better, but maybe not always justified, I would think it would be best to have:

* auto_init_drivers
  
  * auto_init_sensors
  * auto_init_actuators (or auto_init_sensors_actuators)
  * all other drivers where having a distinct category is not yet worth it

I still don't get how this distinction is "worth it". We had this topic already, but feel free to change my mind.
Those comments starting here: #11871 (comment)

I agree we shouldn't have this PR pending much longer. It has good potential.

@fjmolinas
Copy link
Contributor

@maribu can we revive this? from re-reading the discussion current state of the PR was preferred by reviewers so let's move forward as is?

- Added module auto_init_actuators_sensors to auto-initialize actuator
  and sensor drivers
    - Auto-initialization for actuator/sensor driver `foo` will be
      implemented in submodule `auto_init_actuators_sensors_foo`
    - SAUL registration is also performed by that hook, if
      `auto_init_saul` is used
- Pseudo-module `auto_init_sensors_actuators_default` is used to pull in
  sensor and actuator auto_init submodules for all drivers used
- Made `auto_init_saul` depend on `auto_init_sensors_actuators_default`
@maribu maribu force-pushed the auto_init_sensors_actuators branch from b77cdd2 to e1936d8 Compare April 1, 2021 07:37
@maribu
Copy link
Member Author

maribu commented Apr 1, 2021

@maribu can we revive this? from re-reading the discussion current state of the PR was preferred by reviewers so let's move forward as is?

I guess a rebase and two ACKs? The rebase is done :-)

Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK!

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 1, 2021
@kaspar030
Copy link
Contributor

I was about to ACK (and postpone merging for a week to until after sofrt freeze), but seeing the discussion above, would it make sense to directly come up with an XFA based init scheme?

@maribu
Copy link
Member Author

maribu commented Apr 9, 2021

I was about to ACK (and postpone merging for a week to until after sofrt freeze), but seeing the discussion above, would it make sense to directly come up with an XFA based init scheme?

Likely. Also, having to migrate init code isn't fun. Doing two migrations for the cost of one sounds like a sweet deal to me. I try to come up with a scheme till hard feature freeze.

@kaspar030
Copy link
Contributor

I try to come up with a scheme till hard feature freeze.

Don't rush and let's maybe sync, I've many ideas already!

@stale
Copy link

stale bot commented Mar 2, 2022

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.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022
@OlegHahm
Copy link
Member

OlegHahm commented Apr 8, 2022

Damn it - I found this one too late. Is this PR sill up to date anyway?

@stale stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Apr 8, 2022
@maribu
Copy link
Member Author

maribu commented Apr 8, 2022

Actually, let's drop this PR. We better reimplement this with @fabian18's XFA auto init as foundation. Let's see who is the first to open a PR for that :)

@maribu maribu closed this Apr 8, 2022
@maribu maribu deleted the auto_init_sensors_actuators branch January 21, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: SAUL Area: Sensor/Actuator Uber Layer Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Process: needs >1 ACK Integration Process: This PR requires more than one ACK Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.