Skip to content

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Oct 3, 2022

Tokens for being-in-a-thread have only been a comment previously, and came to prominence through #18 and a reddit thread.

As a first demo, the tokens can be used with a Mutex when locking. This does not close #18, as the mutex function stays as it is -- but it might be deprecated, or (compatibly ... how?) changed to return a Result that the user would then need to unwrap (but does that make it better? Might it just be deprecated?).

As they work just the same, it also adds tokens for "being in an interrupt context", although there's probably not much that can be done with them (because interrupts could still be nested).

Advanced usage

More APIs should probably switch from being implemented on an X to being implemented on ValueInThread. I2C transfers come to mind.

APIs that explicitly send data between threads might want to offer ways to also transport ValueInThread. I'm not sure yet whether that's more easily achieved by allowing either trait requirement (is that possible even?), or whether they'd just transport .into_inner() and then use the recipient's knowledge of being in a thread to reconstitute the wrapper.

Alternatives

Rather than having a single wrapper for any T that is not sent to interrupt contexts, it would be possible to have a Mutex type with generic variants (eg. const IS_IN_THREAD: bool).

The largest upside of that alternative is documentation visibility: ValueInThread<&Mutex> is documented with ValueInThread, not with Mutex.

@chrysn chrysn marked this pull request as draft October 3, 2022 08:13
This allows locking a mutex without the overhead of a check whether code
is running in thread mode.

Contributes-To: #18
@chrysn
Copy link
Member Author

chrysn commented Oct 4, 2022

The documentation sittuation doesn't make me happy; the current head ("thread, mutex: Use a mutex-specific referenc type rather than ValueInThread") is an attempt to enhance this.

It makes things only marginally better: The documentation for the "good" .lock() function is still not on the Mutex page.

This could be taken a tad further by moving the Context into the Mutex, but then all of a sudden .with_context() would create a type punning reference through unsafe code -- an artifact of that the context is not, after all, a property of the mutex, but of the reference.

@chrysn
Copy link
Member Author

chrysn commented Oct 4, 2022

I've pushed an extension to the previous approach that uses fundamental -- that's not documented yet but does have the right effect.

I'm rather happy with how that looks in documentation and feels in use, but I'd try it on other don't-call-in-ISR types before committing to it.

@chrysn
Copy link
Member Author

chrysn commented Oct 5, 2022

On the list of "where to try this":

  • msg handles things in a pretty automated fashion. The best that'd be gained here is that from a thread, Msg::send could become infallible, but that'd require a guaranteed-always-valid PID (which we don't have types for yet).
  • I2C devices and similar should probably take an InThread. Not sure right now whether it's better just not made Send (which looks easier on the types), or whether to implement everything on ValueInThread<I2CDevice> (which makes it more straightforward to send an I2C device between threads when needed).
  • All networking stuff: No clue how that'd even react to being called from ISRs.
    [edit: The gnrc::netreg code already takes a msg::v2 SendPort, but that in itself is Send -- so while that's contrived, nothing is keeping a thread from preparing all the parts and then letting gnrc_netreg_register be called in an ISR]

@chrysn
Copy link
Member Author

chrysn commented Jan 25, 2023

With the way ValueInThread is now available, experimentation can go on independently (eg. in #33). The current additions don't break anything, and allow users to compatibly (thanks to Deref) promote their values to ValueInThread already, anticipating changes such as #33.

I'd give this a try.

@chrysn chrysn marked this pull request as ready for review January 25, 2023 00:55
@chrysn chrysn merged commit fef42fa into main Jan 25, 2023
@chrysn chrysn deleted the in_thread_token branch January 25, 2023 00:56
SimonIT pushed a commit to ATACAMA-Project/rust-riot-wrappers that referenced this pull request Oct 9, 2023
Add DHT Wrapper

Closes RIOT-OS#19

See merge request atacama/rust-riot-wrappers!25
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.

Avoid "panics when called in ISR" functions
1 participant