drivers: stepper: refactor ISR safe event mechanism #90424
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025 Jilay Sandeep Pandya | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| zephyr_library() | ||
|
|
||
| zephyr_library_sources(stepper_event_handler.c) | ||
| zephyr_linker_sources(DATA_SECTIONS iterables.ld) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2025 Jilay Sandeep Pandya | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| config STEPPER_EVENT_HANDLER | ||
| bool "Stepper event handler" | ||
| default y | ||
| help | ||
| Enable the dispatch of stepper generated events via | ||
| a message queue to guarantee that the event handler | ||
| code is not run inside of an ISR | ||
|
|
||
| config STEPPER_EVENT_HANDLER_QUEUE_LEN | ||
| int "Maximum number of pending stepper events" | ||
| default 4 | ||
| depends on STEPPER_EVENT_HANDLER | ||
| help | ||
| The maximum number of stepper events that can be pending before new events | ||
| are dropped. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # iterables.ld | ||
| #include <zephyr/linker/iterable_sections.h> | ||
| ITERABLE_SECTION_RAM(stepper_event_handler, Z_LINK_ITERABLE_SUBALIGN) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: Copyright (c) 2025 Jilay Sandeep Pandya | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| #include <zephyr/init.h> | ||
| #include <zephyr/sys/iterable_sections.h> | ||
| #include <zephyr/drivers/stepper.h> | ||
| #include <zephyr/drivers/stepper/stepper_event_handler.h> | ||
|
|
||
| #include <zephyr/logging/log.h> | ||
| LOG_MODULE_REGISTER(stepper_event_handler, CONFIG_STEPPER_LOG_LEVEL); | ||
|
|
||
| int stepper_post_event(const struct device *dev, stepper_event_callback_t cb, | ||
| enum stepper_event event, void *user_data) | ||
| { | ||
| if (cb == NULL) { | ||
| LOG_ERR("Callback is NULL for device %s", dev->name); | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| if (!k_is_in_isr()) { | ||
| cb(dev, event, user_data); | ||
| return 0; | ||
| } | ||
|
|
||
| STRUCT_SECTION_FOREACH(stepper_event_handler, entry) { | ||
| if (entry->dev == dev) { | ||
| struct stepper_event_data data = { | ||
| .event_cb = cb, | ||
| .event = event, | ||
| .user_data = user_data, | ||
| }; | ||
| LOG_DBG("Posting event %d for device %s with cb %p", event, dev->name, | ||
| data.event_cb); | ||
| k_msgq_put(&entry->event_msgq, &data, K_NO_WAIT); | ||
| k_work_submit(&entry->event_callback_work); | ||
| return 0; | ||
| } | ||
| } | ||
| return -ENODEV; | ||
| } | ||
|
|
||
| static void stepper_work_event_handler(struct k_work *work) | ||
| { | ||
| struct stepper_event_handler *entry = | ||
| CONTAINER_OF(work, struct stepper_event_handler, event_callback_work); | ||
| struct stepper_event_data event_data; | ||
| int ret; | ||
|
|
||
| LOG_DBG("Starting event handler for device %s", entry->dev->name); | ||
|
|
||
| ret = k_msgq_get(&entry->event_msgq, &event_data, K_NO_WAIT); | ||
| if (ret != 0) { | ||
| return; | ||
| } | ||
|
|
||
| /* Run the callback */ | ||
| if (event_data.event_cb != NULL) { | ||
| LOG_DBG("Handling event %d for device %s with cb %p", event_data.event, | ||
| entry->dev->name, event_data.event_cb); | ||
| event_data.event_cb(entry->dev, event_data.event, event_data.user_data); | ||
| } | ||
|
|
||
| /* If there are more pending events, resubmit this work item to handle them */ | ||
| if (k_msgq_num_used_get(&entry->event_msgq) > 0) { | ||
| k_work_submit(work); | ||
| } | ||
| } | ||
|
|
||
| static int stepper_event_handler_init(void) | ||
| { | ||
| STRUCT_SECTION_FOREACH(stepper_event_handler, entry) { | ||
| k_msgq_init(&entry->event_msgq, entry->event_msgq_buffer, | ||
| sizeof(struct stepper_event_data), | ||
| CONFIG_STEPPER_EVENT_HANDLER_QUEUE_LEN); | ||
| k_work_init(&entry->event_callback_work, stepper_work_event_handler); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| SYS_INIT(stepper_event_handler_init, APPLICATION, CONFIG_STEPPER_INIT_PRIORITY); | ||
|
There was a problem hiding this comment. This feels off imo, the driver should be ready for use after |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to do this as an iterable section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea is that the event handling data structure is same for all the drivers, hence it could be registered for all the drivers through
STEPPER_DEVICE_DT_INST_DEFINE, sensors also does it, for some other purpose though :)And then via SYS_INIT one could initialize the workq and msgq in terms of event handling for all the drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any drawback of having the
struct stepper_event_handler, as a data member of the stepper driver? Then the members could be initialised during the driver init and at the same time use the common function fromstepper_event_handler.c.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but then each and every driver would have to do this explicitly. And in this way it happens in the background.
Also with CONFIG_STEPPER_EVENT_HANDLER=n, you could switch off this functionality and then the callback would just simply be called without going via the
event_handler