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

sys/bhp_msg: add IPC based implementation of Bottom Half Processor #18464

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Aug 17, 2022

Contribution description

This PR adds an IPC version of the bhp module. It allows device independent IRQ offloading using message queues.
A bhp_msg_t descriptor holds the PID of a thread that performs Bottom Half processing on reception of a message with a dedicated type. Similar to #18435, this mechanism can be adapted to existing init functions of drivers to hook in IRQ offloading.

Testing procedure

tests/unittests should pass.

Issues/PRs references

#18435
Required for adding LWIP support for radios that require IRQ offloading

@jia200x jia200x requested a review from benpicco August 17, 2022 15:18
@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework labels Aug 17, 2022
@jia200x jia200x requested a review from maribu August 17, 2022 15:18
@kaspar030
Copy link
Contributor

This is a bad idea. Messages sent from ISR silently vanish if the receiver is not waiting (in msg_receive()), or if there's a message queue and that is full. Both will happen once there's load, so it's difficult to track down.

The effect would be easy to show with a unit test that calls bhp_msg_isr_cb() twice from an actual ISR. Without a queue, the second message will be lost. Or with a queue of size N, call bhp_msg_isr_cb` N+1 times.

I'm beginning to think we should deprecate msg_send() from ISRs and only allow msg_try_send(), which msg_send() will use anyways when called from ISR, eating the result code.

@jia200x
Copy link
Member Author

jia200x commented Aug 17, 2022

As discussed in many old PRs, I was never a fan of ISR offloading using IPC and I 100% agree with what you describe. I plan to migrate all the remaining IPC based IRQ Offloading modules to Events or Thread Flags at some point.
I only introduce this module to give support for the existing modules (concretely LWIP), since they already use IPC.
IMO it shouldn't make the situation worse but at least it should speed up the Radio HAL migration.

@benpicco
Copy link
Contributor

OK I see, this is needed because LWIP uses msg already for netdev events and this will integrate with it.

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Code looks good, but there should be some explanation as to when bhp_event and when bhp_msg should be used.

CI has some comments too.

sys/bhp/Kconfig Outdated Show resolved Hide resolved
@jia200x
Copy link
Member Author

jia200x commented Aug 18, 2022

done!

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Static tests are still failing

@benpicco
Copy link
Contributor

Please squash!

@jia200x
Copy link
Member Author

jia200x commented Aug 18, 2022

squashed!

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 19, 2022
@benpicco benpicco merged commit 0b5f270 into RIOT-OS:master Aug 19, 2022
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants