pbio/os: Prototype simpler and unified async OS. #298
Conversation
|
Example usage: pbio_error_t pbio_os_wait_example(pbio_os_state_t *state, uint32_t duration) {
static pbio_os_timer_t timer;
ASYNC_BEGIN(state);
printf("Going to wait for %d ms\n", duration);
// We can return errors, parse args etc.
if (duration > 2000) {
return PBIO_ERROR_INVALID_ARG;
}
// This is a nice convenience wrapper I always missed.
AWAIT_MS(state, &timer, duration);
ASYNC_END(PBIO_SUCCESS);
}
pbio_error_t pbio_os_sub_example(pbio_os_state_t *state) {
static pbio_os_state_t child;
static uint32_t counter;
pbio_error_t err;
ASYNC_BEGIN(state);
for (counter = 0; counter < 100; counter++) {
AWAIT(state, &child, err = pbio_os_wait_example(&child, 100 * counter));
if (err != PBIO_SUCCESS) {
printf("Something bad happened!\n");
return err;
}
}
// Return arbitrary error code. Macro is needed to close up the } bracket just like in Contiki.
ASYNC_END(PBIO_SUCCESS);
}
// Outermost "process thread" looks the same but must only have the state argument
pbio_error_t pbio_os_example_thread(pbio_os_state_t *state) {
static pbio_os_state_t child;
pbio_error_t err;
ASYNC_BEGIN(state);
// Await and get/use return value of awaited thread.
AWAIT(state, &child, err = pbio_os_sub_example(&child));
if (err != PBIO_SUCCESS) {
return err;
}
ASYNC_END(PBIO_SUCCESS);
} |
|
Similarly, the following: PT_THREAD(pbdrv_uart_write(struct pt *pt, pbdrv_uart_dev_t *uart, uint8_t *msg, uint8_t length, uint32_t timeout, pbio_error_t *err)) {
PT_BEGIN(pt);
if (!msg || !length) {
*err = PBIO_ERROR_INVALID_ARG;
PT_EXIT(pt);
}
if (uart->tx_buf) {
*err = PBIO_ERROR_BUSY;
PT_EXIT(pt);
}
uart->tx_buf = msg;
uart->tx_buf_size = length;
uart->tx_buf_index = 0;
if (timeout) {
etimer_set(&uart->tx_timer, timeout);
}
uart->USART->CR1 |= USART_CR1_TXEIE;
// Await completion or timeout.
PT_WAIT_UNTIL(pt, uart->tx_buf_index == uart->tx_buf_size || (timeout && etimer_expired(&uart->tx_timer)));
if (timeout && etimer_expired(&uart->tx_timer)) {
uart->USART->CR1 &= ~(USART_CR1_TXEIE | USART_CR1_TCIE);
*err = PBIO_ERROR_TIMEDOUT;
} else {
etimer_stop(&uart->tx_timer);
*err = PBIO_SUCCESS;
}
uart->tx_buf = NULL;
PT_END(pt);
}Becomes: pbio_error_t pbdrv_uart_write(pbio_os_state_t *state, pbdrv_uart_dev_t *uart, uint8_t *msg, uint8_t length, uint32_t timeout) {
ASYNC_BEGIN(state);
if (!msg || !length) {
return PBIO_ERROR_INVALID_ARG;
}
if (uart->tx_buf) {
return PBIO_ERROR_BUSY;
}
uart->tx_buf = msg;
uart->tx_buf_size = length;
uart->tx_buf_index = 0;
if (timeout) {
pbio_os_timer_set(&uart->tx_timer, timeout);
}
uart->USART->CR1 |= USART_CR1_TXEIE;
// Await completion or timeout.
AWAIT_UNTIL(state, uart->tx_buf_index == uart->tx_buf_size || (timeout && pbio_os_timer_is_expired(&uart->tx_timer)));
uart->tx_buf = NULL;
if (timeout && pbio_os_timer_is_expired(&uart->tx_timer)) {
uart->USART->CR1 &= ~(USART_CR1_TXEIE | USART_CR1_TCIE);
return PBIO_ERROR_TIMEDOUT;
}
ASYNC_END(PBIO_SUCCESS);
}Error handling at the start and towards the end looks quite a bit more elegant. Awaiting on the condition remains basically the same. |
|
This also lets us eliminate wrapper threads such as these: PROCESS_THREAD(pwm_lp50xx_stm32, ev, data) {
PROCESS_BEGIN();
// need to allow all drivers to init first
PROCESS_PAUSE();
for (;;) {
for (int i = 0; i < PBDRV_CONFIG_PWM_LP50XX_STM32_NUM_DEV; i++) {
pbdrv_pwm_lp50xx_stm32_priv_t *priv = &dev_priv[i];
pbdrv_pwm_lp50xx_stm32_handle_event(priv, ev);
}
PROCESS_WAIT_EVENT();
}
PROCESS_END();
}Instead, we can start I've added this example to this PR. It looks like a slightly bigger change, but that is only because I moved |
laurensvalk
force-pushed
the
os
branch
2 times, most recently
from
c7394fa to
05946cf
Compare
|
Any objections or thoughts @dlech ? If not, we could merge and gradually update each process without breaking things. |
The goal is to: - Add error return value to all protothreads. - Reduce complexity and code size. - Avoid risk of overruning event queue. - Use single poll flag and no broadcasting between threads. - Cross-platform handling of events during sleep. - Make stm32, ev3, nxt all work the same way. For now the new OS also drives the contiki event loop so we can migrate processes one by one instead of breaking everything at once.
Idling with WFI was previously implemented in the MicroPython HAL. Now that we moved it to pbio/os in a platform agnostic way, we can fix this longstanding open REVISIT note.
This is a simple example to show that we can convert the processes one at a time.
When using the same thread for multiple processes it can be useful to have a reference to the state that belongs to that instance. This avoids having to use several cases of CONTAINER_OF to get to the parent of a state, which is not always available. This also brings it one step closer to pbio tasks, which we might model in this way too.
Similar to PROCESS_PAUSE() but without posting an event to itself like Contiki does. It is basically the same as awaiting 0ms, but without the need to allocate a timer. Also restore this for the charger, which used PROCESS_PAUSE() here initially.
Otherwise we don't drive the new processes.
Otherwise we don't drive the new processes. Fixes the light not blinking at the end because the newly updated LED process was not driven during pbsys deinit.
|
I haven't had time yet to have a close look at this yet. If you can wait a bit longer, I can try to have a look this weekend. |
|
Sure, no rush -- that's what PRs are for 🙂 Thanks! |
|
I assume this is the correct place to report: Running All blinking I talk about here is blue. To reproduce:
Situation 1. occurs more often: 1 in ~4 to 10 attempts Testing with bluetooth, so I often disconnect for BT to be stable. I did not succeed in a more stable scenario. No need to give this a priority, I just wanted to report it. Bert [EDIT] [EDIT 2] |
dlech
left a comment
dlech
left a comment
There was a problem hiding this comment.
This stuff is tricky to get right. We will probably introduce lots of bug/regressions by doing this. But in the end, it will be simpler/nicer. So probably worth it in the long run.
Thanks for the overal and specific feedback! |
The intention is to use it like this: pbio_error_t example_thread_func(pbio_os_state_t *state) {
pbio_error_t err;
ASYNC_BEGIN(state);
...
AWAIT(state, &child, err = pbdrv_uart_write(&child, device, data, len, timeout));
if (err != PBIO_SUCCESS) {
return err;
}
...
ASYNC_END(PBIO_SUCCESS);
}The goal here is indeed precisely to handle errors. The
The example above mostly achieves this, but if there is a clever way to make the macros work differently, we could look at it. Right now they work just like Contiki. |
Yes, this is one of the main design considerations we'll want to think about thoroughly before going ahead. It simplifies processes a lot when everything is working, but debugging could be harder. Maybe we could have the auto-polling be optional for each thread. Then certain processes can independently (self-)poll as we used to do.
This is one of the things that caught me out in the Contiki way of doing things -- if it got to a yield-once-come-back-later check, it could "miss" the event if it had just got it before getting there. In the new proposal, this should be simpler. What cases are you thinking of that could now be trickier? |
Yeah, this is probably only to be used for certain special cases. It is something I'd like to use for the Basically, it "detaches" the LEGO DCM+LUMP process when the port goes into a different mode. It could attach that "none_process" dummy process instead if that mode needs no process. This allows everything to be synchronously handled rather than having |
Can be called from interrupt handlers.
|
Everything I saw only returned Adding a |
|
Errors are passed on everywhere, including in the PT_THREAD(pbdrv_uart_write(struct pt *pt, pbdrv_uart_dev_t *uart, uint8_t *msg, uint8_t length, uint32_t timeout, pbio_error_t *err)) {
PT_BEGIN(pt);
if (!msg || !length) {
*err = PBIO_ERROR_INVALID_ARG;
PT_EXIT(pt);
}
if (uart->tx_buf) {
*err = PBIO_ERROR_BUSY;
PT_EXIT(pt);
}
...to: pbio_error_t pbdrv_uart_write(pbio_os_state_t *state, pbdrv_uart_dev_t *uart, uint8_t *msg, uint8_t length, uint32_t timeout) {
PBIO_OS_ASYNC_BEGIN(state);
if (!msg || !length) {
return PBIO_ERROR_INVALID_ARG;
}
if (uart->tx_buf) {
return PBIO_ERROR_BUSY;
}
...The |
That is what I was trying to suggest in #298 (comment) 😉 If we drop the do/while, the #define AWAIT(state, child, expression) { \
ASYNC_INIT((child)); \
PB_LC_SET(state); \
pbio_error_t _err = expression; \
if (_err== PBIO_ERROR_AGAIN) { \
return PBIO_ERROR_AGAIN; \
} \
_err; \
}pbio_error_t err;
...
err = AWAIT(some_protothread())
if (err)
return err;
...I guess we would have to introduce a struct with two |
lib/pbio/include/pbio/os.h
Outdated
| * @param [in] child Protothread state of the child. | ||
| * @param [in] statement The statement to await. | ||
| */ | ||
| #define AWAIT(state, child, statement) \ |
There was a problem hiding this comment.
Technically, this need to be an expression, not a statement. And the doc comment needs to say that the expression needs to evaluate to a pbio_error_t. Same applies to the AWAIT_RACE as well.
|
Very interesting! The expression statement is a nice way of defining a local |
Generalizes handling to work better for the virtual hub, which doesn't use uint32_t for the interrupt state.
This was initially removed in #298, but we are restoring it to be sure that it isn't a breaking change.
We can fix:
all at once by having these hooks in a dedicated |
Proceed to make pbio more independent from MicroPython. Also restore them as static inline as they used to be prior to #298 which reduces build size.
That looks really nice, but it appears we cannot jump back into expression statements: |
|
Most points here have been addressed. Thanks for the feedback. I'll move some of the
This is the slightly more fundamental one that is remaining. I'll think about it some more before we merge this. |
|
I think we can merge this and develop from here. Contiki will stay around for the foreseeable future so we can upgrade processes to the new pattern one by one without breaking things, or easily find where things did break. I have not yet tested @BertLindeman's findings. Size reductions probably won't be apparent until that time, but we have a bit of space to spare for now. |
Will use firmware of commit 0663e76 to test |
|
A few more relevant changes have also been merged, so you might want to pick the latest action / nightly build. Thanks! |
|
Tested with Still see strange things: Connected via BT to the UI (beta), press disconnect bluetooth in the UI and look at the hub light:
hub.switches.off.mp4
hub.waits.before.BT.pairing.mode.mp4
[EDIT] Make a new issue? [EDIT2] |
|
Thanks @BertLindeman. I'll keep an eye out for these two as we continue to develop. We're making rapid changes at the moment, so it's certainly possible that some issues pop up. I think this in particular is an area we'll be reviewing more closely in the future. |
|
So happy that Pybricks 3 runs on EV3! |
|
We're of a mind to call it Pybricks 4.0 by the time we get there =) |
|
@BertLindeman, are you still seeing any of these issues with the latest nightly builds? Many thanks! |
|
will look at it 😃 |
|
Refer to remark above All tests in this post with firmware at hash dfbcfa8 (nightly build 3821) Problem 1 went away: was
Now: hub goes after a short wait to blink-blink-pause So OK..
Redid these tests with the same cityhub and now not always a problem. [WIll update this post while also testing other hubs] |
|
tests with Short for "blink-blink-pause": b-b-p
Test program for 5: from pybricks import version
print(f'version {version}')Remark on 5: if the program differs from this print version, also on test 5 the cityhub re-powers-on, from pybricks import version
from pybricks.tools import wait
print(f'version {version}')and even re-powers-on if the program is as short as this: print('version')
[Dinertime now] |
|
Small thing (to me): Normally an idle hub keeps trying for some time to BT connect after the gui disconnects. But, scenario:
If I wanted to shutdown the hub, I would use the hub button (if I can reach it) |
Overall, the goals are to reduce complexity, reduce code size, and make it more cross platform.
In this PR, the new approach still drives the original
pbioevent loop as well, so we can convert one process at a time without breaking things. The charger process is done here as a simple test to illustrate what it looks like. Once all processes have been converted and everything is working, we could drop the Contiki dependency.Not everything we need is implemented yet, but it is already easy to see that
pbio/osis much easier to get into than the full contiki library. Mainly because we only implement what we need, but also due to the simplifications introduced below.Returning errors from protothreads is currently cumbersome. This modification allows protothreads to return error codes.
PBIO_ERROR_AGAINessentially takes the role ofPT_YIELDED/PT_WAITING. Everything else indicates completion with the respective error or success.Contiki partially distinguishes (but not really) between waiting and yielding. In practice, both yield and the latter yields at least once. And then both macros exist separately for processes and protothreads so you end up with four variants that are almost the same but subtly different. I think we can do everything with a single set of
AWAITmacros.The contiki event timers are more complicated than we need for our use case. Currently, a timer tick first polls the timer process and then posts N events depending on the number of active timers. This is not be safe if we have more timer events than fit in the event queue. Also, etimers use the global
PROCESS_CURRENTwhich requires hacks such asPROCESS_CONTEXT_BEGINwhen accessing them from elsewhere.Instead of using various events and polling individual processes throughout, I think we may be able to work with just a single
pbio_os_request_poll()function. Then all processes will check their current wait condition. This should not be that much more computationally expensive than checking theneeds_pollflag on each process (and it may be less due to overall simplicity).We currently use broadcasting between processes in a few places, which can be hard to follow and risks overflowing the event queue as already stated in a few existing
REVISITs around the code. We have recently eliminated this for the uart process. We should be able to do this for the status change too.We can move some of the logic for handling pending events and entering sleep mode to
pbioinstead of having it inmphalport. Aside from hooks such asenable_irqit becomes platform agnostic. This also brings the virtual hub a bit closer to the embedded hubs. It also unifies the EV3 and NXT with Powered Up.