Display methods don't require critical section token
videbar opened this issue · 7 comments
The documentation of some methods from the non-blocking Display struct state that they must be called from inside a critical section, however, this is not enforced by requiring a critical section token as an argument.
I've noticed this on the following methods:
I think the methods should require the critical section token, however, changing the methods' signature would be a breaking change.
An alternative would be to add a "safe" version of each of the previous methods that implements the safe functionality but requires the token (for example safe_show()) and mark the current versions as deprecated.
If there's an agreement on a solution, I could work on this and open a PR.
I believe Display doesn't access any resources it doesn't own.
Those methods all take a mutable reference to a Display, so there should be no risk of more than one of them being called at a time.
So I'm not sure what restriction those "This must be called from within a critical section" notes were intended to convey. I don't think any further restriction should be needed.
In practice I think you'd usually set things up so that you have a single Display instance that you can only get a reference to when you're inside some kind of critical section (for example using the techniques described at Sharing Peripherals), but that isn't strictly Display's business.
I've reading the documentation some more and, so far, my best guess is that the restriction may refer to this:
You can call
show()at any time, so long as you’re not interrupting, or interruptable by,Display::handle_display_event().
Is it possible that they goal is to prevent the timer driving the display from interrupting, or being interrupted by, the show(), show_frame(), or clear() methods?
I think that's the same concern. The timer driving the display will do so by calling handle_display_event(), which again takes Display by mutable reference.
As long as you're writing safe code, there's no way you could be breaking this rule. At any rate I don't think adding a critical section token would give us any further safety.
Possibly that "must not be interrupting, or interruptable by" text should never have been there.
OK, I've found a note I made around the time I finished off Display.
Does it make any sense now to have the text saying "The code that calls this method must not be interrupting, or interruptable by,
handle_event()?"Or does it go without saying now they're both methods of one object?
I think I'm happy to leave it as it is for now. As there's no standard framework I don't think it entirely goes without saying that you can't overlap two method calls on the same object.
So if this text is to remain, I think it should be understood as advice for people writing unsafe code. But right now I feel it would be less confusing to remove it.
Cool, thanks for looking into this. So I guess you were right and, as long as the code is safe, the borrow checker takes care of the necessary guarantees.
I would suggest replacing this
This must be called from within a critical section.
by something like what you said, for example
In most cases a single Display instance is used, which is shared across interrupts and other concurrent parts of the program. Because of this, this method is usually called from inside a critical section.
Or simply removing it altogether.
So if this text is to remain, I think it should be understood as advice for people writing unsafe code. But right now I feel it would be less confusing to remove it.
Agreed, an alternative can be to make it explicit that one should only think about this when writing unsafe code. Something like:
You can call
show()at any time, so long as you’re not interrupting, or interruptable by,Display::handle_display_event(). This should only be a consideration when writing unsafe code, otherwise the borrow checker ensures that this requirement is fulfilled.
Agreed, an alternative can be to make it explicit that one should only think about this when writing unsafe code. Something like:
You can call
show()at any time, so long as you’re not interrupting, or interruptable by,Display::handle_display_event(). This should only be a consideration when writing unsafe code, otherwise the borrow checker ensures that this requirement is fulfilled.
I like this alternative
I had forgotten about this issue, but I've just opened a PR updating the documentation. Feel free to suggest any changes to the wording.