Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PoC] Reactive SAUL: saul_observer based on callbacks #16277

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

jue89
Copy link
Contributor

@jue89 jue89 commented Apr 4, 2021

Contribution description

After almost a year - sorry for the long delay - I finally managed to put some time into the promised alternative implementation for #14121. This PR is a proof-of-concept to play around with the concept. With some aftermath, this can be cleaned up for a merge into master.

The main differences to #14182:

  • It's based on callbacks that are put into a linked list for each SAUL device. The idea is to implement a simple callback that kicks-off your favorite IPC mechanism. For convenience, I've already added helpers for sending messages and waking up threads (cf. utils.c)
  • I've improved the handling of SAUL device events: If a device changed its state, the device will be put into an event queue, if it's not already in that queue. This ensures that every event is handled - at least if the previous event has been processed before. In the other PR, I've used msg_t which may fail to be delivered.
  • The check function inside of saul_driver_t isn't mandatory anymore. If it's not implemented (i.e. NULL), every SAUL event will make it to the registered callbacks. This way, every SAUL_ACT_* is observable by default! If it's being written, observers will be notified! I added a flag that can be returned by the read and write implementation indicating that the check function shall be called.

Testing procedure

Flash tests/saul_observer onto a board with saul_gpio support and play around with the registered SAUL devices. You should see some change notifications.

# make -C tests/saul_observer BOARD=samr30-xpro flash term
2021-04-04 11:08:30,140 # main(): This is RIOT! (Version: 2021.04-devel-1239-g40837-feature/saul_observer_callback)
2021-04-04 11:08:30,142 # SAUL observer test application
2021-04-04 11:08:30,145 # Register SAUL device LED(green)
2021-04-04 11:08:30,148 # Register SAUL device LED(orange)
2021-04-04 11:08:30,151 # Register SAUL device Button(SW0)
> saul
2021-04-04 11:09:09,971 # saul
2021-04-04 11:09:09,973 # ID	Class		Name
2021-04-04 11:09:09,975 # #0	ACT_SWITCH	LED(green)
2021-04-04 11:09:09,977 # #1	ACT_SWITCH	LED(orange)
2021-04-04 11:09:09,980 # #2	SENSE_BTN	Button(SW0)
> saul write 0 1
2021-04-04 11:09:14,953 # saul write 0 1
2021-04-04 11:09:14,956 # Writing to device #0 - LED(green)
2021-04-04 11:09:14,958 # Data:	              1 
2021-04-04 11:09:14,961 # data successfully written to device #0
> 2021-04-04 11:09:14,966 # SAUL DEV LED(green) CHANGED - Data:	              1 
2021-04-04 11:09:19,949 # SAUL DEV Button(SW0) CHANGED - Data:	              1 
2021-04-04 11:09:19,986 # SAUL DEV Button(SW0) CHANGED - Data:	              0 

Issues/PRs references

#14121
#14182

@jue89 jue89 added Area: SAUL Area: Sensor/Actuator Uber Layer Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Apr 4, 2021
@benpicco benpicco requested a review from chrysn April 5, 2021 21:31
@jue89
Copy link
Contributor Author

jue89 commented Apr 11, 2021

After a week, I've successfully built a system based on this driver:

  • I've built a server that exposes all SAUL devices over a hacked-together, fully works-for-me-driven UDP/CBOR protocol. It runs on a RIOT-OS node which has 3 switches and LEDs as the user interface and a DOSE network interface for inter-connection.
  • Furthermore, I've built a client running on a Raspberry Pi. It receives UDP datagrams from the server over multicast for a) discovery of the node b) meta information about all exposed SAUL devices (name, type, readable, writable) and c) state changes. And it sends UDP packets if a writable SAUL device shall change its state. The little discovery example shows pretty well, what steps it take to observe all SAUL devices exposed to the network:
    • Start up the client. It binds to port 5001 and joins the multicast group ff02::cafe.
    • Listen for discover events. They will be raised once a new RIOT-OS node has been found. This happens after 20 seconds max, as all nodes periodically publish their states.
    • After a device (i.e. RIOT-OS node) has been discovered, get all its endpoints (i.e. SAUL devices), print the device's CPUID, the endpoint's name (this combination is used to address SAUL devices on the network) and the current state. Additionally, listen for change events. Every time, the SAUL device changes its state, this will be displayed instantaneously.

The main benefit brought to me by this driver is the ability to get notified on SAUL device state changes (i.e. someone pressed that switch ...). I can react on this event and turn on the LED for instance.

I'm going to test flight this system in my home automation setup. I wrote an adapter to the client mentioned above. The switch es are now glued to the desk and can be used to turn on the printer and the PC. My flatmates are already making fun of me, since the project turn on the printer and a LED by pressing a button took me way tooo long :D

The switches under the desk

The switches under the desk

The Raspberry Pi with its network interface attached and covered with too much dust (sry!)

The Raspberry Pi with its network interface attached

@jue89 jue89 added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Apr 12, 2021
@jue89
Copy link
Contributor Author

jue89 commented Apr 14, 2021

I extended the DHT sensor driver to demonstrate how an analog sensor - in this case temperature and humidity - could be made observable. It uses the .check method implemented by the saul_driver_t to check if temperature and humidity have changed significantly.

Works like a charm:

2021-04-14 19:40:07,019 # main(): This is RIOT! (Version: 2021.04-devel-1244-g3d24d-feature/saul_observer_callback)
2021-04-14 19:40:07,020 # SAUL observer test application
2021-04-14 19:40:07,020 # Register SAUL device LED
2021-04-14 19:40:07,020 # Register SAUL device KEY
2021-04-14 19:40:07,021 # Register SAUL device dht
2021-04-14 19:40:07,021 # Register SAUL device dht
> 2021-04-14 19:40:07,021 # SAUL DEV dht CHANGED
2021-04-14 19:40:07,022 # Data:	           20.8 °C
2021-04-14 19:40:07,022 # SAUL DEV dht CHANGED
2021-04-14 19:40:07,022 # Data:	           60.7 %
2021-04-14 19:40:16,986 # SAUL DEV dht CHANGED
2021-04-14 19:40:16,987 # Data:	           66.7 %
2021-04-14 19:40:19,986 # SAUL DEV dht CHANGED
2021-04-14 19:40:19,987 # Data:	           72.2 %
2021-04-14 19:40:22,986 # SAUL DEV dht CHANGED
2021-04-14 19:40:22,987 # Data:	           77.2 %
2021-04-14 19:40:27,987 # SAUL DEV dht CHANGED
2021-04-14 19:40:27,987 # Data:	           82.7 %
2021-04-14 19:40:44,987 # SAUL DEV dht CHANGED
2021-04-14 19:40:44,987 # Data:	           77.0 %
2021-04-14 19:40:50,987 # SAUL DEV dht CHANGED
2021-04-14 19:40:50,987 # Data:	           71.7 %
2021-04-14 19:41:00,486 # SAUL DEV dht CHANGED
2021-04-14 19:41:00,486 # Data:	           66.4 %
2021-04-14 19:41:25,486 # SAUL DEV dht CHANGED
2021-04-14 19:41:25,487 # Data:	           61.4 %
2021-04-14 19:42:39,487 # SAUL DEV dht CHANGED
2021-04-14 19:42:39,488 # Data:	           67.3 %
2021-04-14 19:42:41,487 # SAUL DEV dht CHANGED
2021-04-14 19:42:41,488 # Data:	           21.8 °C
2021-04-14 19:42:54,488 # SAUL DEV dht CHANGED
2021-04-14 19:42:54,488 # Data:	           22.8 °C
2021-04-14 19:43:00,488 # SAUL DEV dht CHANGED
2021-04-14 19:43:00,488 # Data:	           23.8 °C
2021-04-14 19:43:13,488 # SAUL DEV dht CHANGED
2021-04-14 19:43:13,489 # Data:	           24.8 °C
2021-04-14 19:43:15,488 # SAUL DEV dht CHANGED
2021-04-14 19:43:15,489 # Data:	           62.2 %
2021-04-14 19:43:27,987 # SAUL DEV dht CHANGED
2021-04-14 19:43:27,987 # Data:	           57.1 %

@jue89 jue89 force-pushed the feature/saul_observer_callback branch from 5eee0c9 to 008352f Compare April 17, 2021 17:10
@jue89
Copy link
Contributor Author

jue89 commented Apr 18, 2021

Cross post from #14121


I think my proof of concept is in a good state for talking about its architecture. I try to give you an overview. It should help to get into its code and design:

saul_observer is powered by its own thread event_thread. Its interface can be divided into the front- and backend. The frontend is used by the application interested into SAUL device changes and the backend allows SAUL device drivers to tell saul_observer about changed devices.

Frontend

  • Every SAUL device (saul_reg_t) maintains a linked-list of observers.
  • An application can become an observer by:
    1. initializing an observe handle (saul_observer_t); i.e. setting a pointer to a callback function (void saul_cb(saul_reg_t *dev, void *arg)) and an optional argument (void *arg)
    2. calling saul_observer_add(saul_reg_t *dev, saul_observer_t *observer).
  • Once an event occurs, the callback function is called in the context of the event_thread thread.
  • The PoC also introduces some convenience functions to support several RIOT IPC systems:
    • saul_observer_msg(saul_reg_t *dev, saul_observer_t *observer, msg_t *msg, kernel_pid_t target) sends msg to target once the SAUL dev changed. The message's content contains a pointer to dev.
    • saul_observer_wakeup(saul_reg_t *dev, saul_observer_t *observer, kernel_pid_t pid) wakes thread pid upon events on dev.
    • saul_observer_set_flag(saul_reg_t *dev, saul_observer_t *observer, kernel_pid_t pid, thread_flags_t flag) sets flag of thread pid.
    • saul_observer_msg_bus(saul_reg_t *dev, saul_observer_t *observer, msg_bus_t *msg_bus) sends a message containing a pointer to dev on the message bus msg_bus. The event type is dev->driver->type % 32.
    • saul_observer_mutex(saul_reg_t *dev, saul_observer_t *observer, mutex_t *mutex) unlocks mutex in case of an event.

Backend

  • saul_observer must be notified if an event occurs. There are three different ways to do so:
    • saul_observer_queue_event(saul_reg_t *dev) can be called. This may happen in an ISR detecting changes of SAUL device dev.
    • The saul_driver_t implementation int read(void *dev, phydat_t *res) may return SAUL_FLAG_QUEUE_EVENT along with the dimension count of phydat_t. For example: return 3 | SAUL_FLAG_QUEUE_EVENT;.
    • The saul_driver_t implementation int write(void *dev, phydat_t *res) also may return SAUL_FLAG_QUEUE_EVENT.
  • Every event adds the saul_reg_t in question to the saul_observer event queue.
  • For every queued event, the corresponding saul_driver_t is checked whether the newly introduced int check(void *dev) is implemented (i.e. the function pointer is non-NULL). If the check function is present, it is called and can check if the queued event shall be propagated to the frontend. This comes in handy if the SAUL devices measure analog values with noise. The check function can suppress events if the causing change is not significant.

To evaluate the concept, I've ported two drivers (saul_gpio and dht) to saul_observer.

I'd like to hear your opinion on this design. At the current stage I need to know if I'm heading in the right direction. If I'm not the only who believes this is a good way to solve the problem we were bikeshedding about, I'm going polish the PoC and bring it into RIOT with some PRs. The first one will be saul_observer. Follow-ups introduce the convenience functions and the driver implementation I've already implemented.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

saul_observer is powered by its own thread.

Isn't it possible to use event_thread for this? Each additional thread needs its own stack. You can make the event queue configurable to allow users to still run it in a dedicated thread, if the three-level priority management of event_thread is to course grained for a specific use case.

Comment on lines 181 to 182
else
FEATURES_REQUIRED += periph_gpio
endif
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
else
FEATURES_REQUIRED += periph_gpio
endif
endif
FEATURES_REQUIRED += periph_gpio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jue89
Copy link
Contributor Author

jue89 commented Apr 20, 2021

Isn't it possible to use event_thread for this? Each additional thread needs its own stack. You can make the event queue configurable to allow users to still run it in a dedicated thread, if the three-level priority management of event_thread is to course grained for a specific use case.

Great idea! TBH I wasn't aware of the existence of event_thread. This reduced the core implementaion of saul_observer to 50 LOC!

For the PoC I'm going for hard-coded EVENT_PRIO_MEDIUM. But making the event queue configurable is probably a good idea.

@jue89
Copy link
Contributor Author

jue89 commented Jun 9, 2021

Are there any further remarks or comments on the proposed architecture?
If not, I'm going to open a PR bringing the saul_observer core into RIOT.

@MrKevinWeiss MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021
@github-actions github-actions bot added Area: doc Area: Documentation Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework labels Jul 1, 2021
@jue89 jue89 force-pushed the feature/saul_observer_callback branch from 2bf0d5f to fb2e305 Compare July 1, 2021 16:06
@MrKevinWeiss MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021
@fjmolinas
Copy link
Contributor

Are there any further remarks or comments on the proposed architecture? If not, I'm going to open a PR bringing the saul_observer core into RIOT.

Please do so!

@stale
Copy link

stale bot commented Jun 12, 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 Jun 12, 2022
@jue89 jue89 removed the State: stale State: The issue / PR has no activity for >185 days label Jun 15, 2022
@Teufelchen1 Teufelchen1 marked this pull request as draft March 26, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: drivers Area: Device drivers Area: SAUL Area: Sensor/Actuator Uber Layer Area: sys Area: System Area: tests Area: tests and testing framework Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants