Skip to content

Conversation

@laurensvalk
Copy link
Member

@laurensvalk laurensvalk commented Mar 19, 2025

We never quite adopted events extensively across pbio. We only have a status set/clear handler in pbsys, with only one top level event listener at pbsys_hmi_handle_status_change. We can call it directly without casting the event data value to void * onto an event queue, and then back again. Given the limited case we had, this was harder to follow than we ultimately needed, and risked overflowing the event queue as stated in an old REVISIT that we can now address.

Doing this simplifies the introduction of #298 by ensuring we don't need any event queue at all.

@coveralls
Copy link

coveralls commented Mar 19, 2025

Coverage Status

coverage: 57.277% (+0.4%) from 56.919%
when pulling cef8586 on status
into 5a5cccf on master.

@laurensvalk laurensvalk force-pushed the status branch 2 times, most recently from 657fa3d to 6102eef Compare March 19, 2025 11:50
@dlech
Copy link
Member

dlech commented Mar 23, 2025

I think this will be OK, but it likely needs more work. I'm pretty sure that we have some processes that are depending on these broadcast events to "wake up" the process when the status changes even if they aren't explicitly checking the event. The main one that comes to mind is Bluetooth/USB, e.g. when the user program stops or when we get a hub shutdown request. Basically, we need to audit everywhere pbsys_status_test() is called an make sure we still have something that is polling that process when the status changes.

@laurensvalk
Copy link
Member Author

Thanks for the review!

I think this will be OK, but it likely needs more work. I'm pretty sure that we have some processes that are depending on these broadcast events to "wake up" the process when the status changes even if they aren't explicitly checking the event.

This is one of those cases where #298 should really simplify things.

Where this code used to broadcast, it can call pbio_os_request_poll to achieve the same.

We can wait with this one until #298 is merged.

This refactors a few names without changing any code, setting the stage for the next commit where we update the sys background process.

We never quite adopted events across pbio, but only had a status set/clear handler in pbsys, with only one top level handler at pbsys_hmi_handle_status_change. Later, we can call it directly without casting the event data value to void * and back.
This was using broadcast, but there was ultimately only one receiver for this data. We can simplify this and avoid the risk of overrunning the event queue by calling it synchronously, which is safe for this event.

After the refactoring in the previous commit, this is now a trivial change.
…nge.

We have already updated the parts that handle status change events, but there are still some processes that await status changes by asynchronously polling.

This change ensures we keep doing that until all processes have been converted to the new pattern.
@laurensvalk
Copy link
Member Author

laurensvalk commented Apr 8, 2025

I think this will be OK, but it likely needs more work. I'm pretty sure that we have some processes that are depending on these broadcast events to "wake up" the process when the status changes even if they aren't explicitly checking the event. The main one that comes to mind is Bluetooth/USB, e.g. when the user program stops or when we get a hub shutdown request. Basically, we need to audit everywhere pbsys_status_test() is called an make sure we still have something that is polling that process when the status changes.

We can address this for now by maintaining the Contiki broadcast event in the status change until all processes that poll the status have been updated. This does not have to broadcast any data, just wake all processes up. So this is easy to remove later.

@laurensvalk laurensvalk merged commit cef8586 into master Apr 8, 2025
32 checks passed
@dlech dlech deleted the status branch April 8, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants