search

sammhicks / nostd_async

An asyncronous runtime for a single-threaded no_std environment
MIT License
17 stars 2 forks source link

Futures are always polled inside a critical section #3

Closed pmarcelll closed 1 month ago

pmarcelll commented 1 year ago

I recently started playing with an Atmega8 MCU, which uses the AVR instruction set and has only 8KB of program memory, but despite this fact I still wanted to try async Rust on it, which led me to this crate. I implemented the missing pieces in my fork, but the new code in the simulator I use (called simavr) stopped receiving interrupts when I tried to run a single, long running Task. After a short investigation I found that here the TaskCore::run_once‎ method, which contains the actual polling, will run inside a critical section.

I tried to work around this issue and I think I came up with a working solution: I added a new struct that can store a reference to first in the linked list, and an instance of this struct can escape the critical section. Dereferencing the reference by calling first.with(...) is still unsafe (but will consume the instance), so practically I just moved the unsafe block and the responsibility into Runtime::run_once. I also put a second, separate critical section into TaskCore::run_once‎ just for self.remove(cs), this way the polling can run after we're done with manipulating the linked list.

What do you think of this solution? (Code comparison here. I also had to replace the bare-metal crate with its successor, critical-section, due to breaking changes in bare-metal 1.0, which is used by avr-device but not by cortex-m. critical-section is optionally supported by both.) Do you see any potential issues with this approach? I'm also happy to open a PR.

sammhicks commented 1 year ago

Based on your design, I've changed the scheduling algorithm to pop tasks from the front of the queue before polling them, allowing the pop to run in a critical section but the polling to run with interrupts enabled, thus eliminating a potential race condition that was in fact already present in my existing design! So, thanks for leading me to find that :) !

Also, as per your suggestion, I've migrated from bare-metal to critical-section, which in fact, is a design that I prefer!

The code's currently sitting in the branch avr_support, I don't have an AVR device handy, so would you mind giving it a go before I merge it?

sammhicks commented 1 month ago

Merged and Closed due to inactivy