Repository navigation
Add click callback for desktop driver - #109
Conversation
|
Hi, thanks @DaneSlattery! I'll try to review this in more detail when I have a chance this week. Cheers 💜 |
|
Sure. My main use case is treating the desktop renderer as a digital twin of my embedded system. The recently added button support allows external controls to be simulated. I would also like each rendered LED to act as a clickable input. My embedded application displays real-world data on its LEDs, so selecting an LED in the simulator lets user code inject or manipulate the corresponding state and observe how the system responds without requiring the physical hardware. This could also enable other workflows in the future. For example, desktop interactions could be forwarded to a physical display over a desktop-to-microcontroller communication channel, or used to create “drawings” For this initial contribution, I’m deliberately proposing a small API: a one-shot event containing the index of the clicked LED. Richer interactions such as press, release, or drag could be added separately if needed. |
| impl<Dim, Layout> DesktopDriver<Dim, Layout> { | ||
| /// Returns a handle for receiving LED click events while this driver is in use by a control. | ||
| pub fn led_clicks(&self) -> LedClickReceiver { | ||
| self.led_click_receiver.clone() |
There was a problem hiding this comment.
Is this needed? Genuine question, wondering whether you use this in your own code.
Because this means we need to clone the receiver, which kinda breaks the "multi-producer single-consumer" abstraction, since we no longer have a single consumer. And so we have to use an Arc Mutex, which breaks the point of using channels.
If we do need this, we should consider a different abstraction for how we send the clicks around. Either a channel that can do single-producer multi-consumer (a broadcast), or, maybe we think about the consumer subscribing and getting their own mpsc channel.
There was a problem hiding this comment.
Do you want to try seeing what happens if you use https://github.com/gilvir/ringcast ?
There was a problem hiding this comment.
A major flaw here is that multiple receivers would contend on a receiver yes. I think a pub-sub model makes more sense. I can look at ringcast, but we could also just use mpmc
There was a problem hiding this comment.
I do think getting rid of the arc mutex is good, as long as the user takes the receiver before starting up the other thread
There was a problem hiding this comment.
we could also just use mpmc
Looks like it's experimental, i.e. not supported in Rust stable?
There was a problem hiding this comment.
Ok I gave ringcast a whirl. The complexity comes in that in order to add more receivers, the Driver would need to own the Sender, but we need the Stage to own the Sender in order to send :-(
We could make it such that the Desktop struct owns the Sender until start, where after it passes it into the Stage, but I'm starting to wonder if multiple consumers are even necessary. I've refactored it so that we don't clone the receiver.
If a user wants to multi-cast the clicks, they can fan out the led clicks into their own multi-consumer channel.
We can also go the desktop_button route with an api like this:
fn main() {
let mut led_clicks = DesktopLedClicks::new();
Desktop::new_1d::<StripLayout>()
.with_led_clicks(&led_clicks)
.start(move |driver| {
let mut control = ControlBuilder::new_1d()
.with_layout::<StripLayout, { StripLayout::PIXEL_COUNT }>()
.with_pattern::<Rainbow>(RainbowParams::default())
.with_driver(driver)
.with_frame_buffer_size::<{ StripLayout::PIXEL_COUNT }>()
.build();
loop {
while let Some(led_index) = led_clicks.try_take() {
println!("LED {led_index} clicked");
// Update application/pattern state here.
}
if let Err(DesktopError::WindowClosed) = control.tick(elapsed_in_ms()) {
break;
}
}
});
}There was a problem hiding this comment.
We can also go the
desktop_buttonroute with an api like this:
That seems better? What do you think?
There was a problem hiding this comment.
Yes I like it! Done
| //! // Create the Desktop simulator | ||
| //! Desktop::new_2d::<PanelLayout>().start(|driver| { | ||
| //! // Keep this receiver before moving the driver into the control. | ||
| //! let led_clicks = driver.led_clicks(); |
There was a problem hiding this comment.
Oh yeah, I see it used here, okay sweet. Hmm...
| /// Returns the next LED index clicked in the simulator, if one is pending. | ||
| /// | ||
| /// This method never blocks. Only successful primary-button LED picks emit events. | ||
| pub fn try_take_led_click(&self) -> Option<usize> { |
There was a problem hiding this comment.
Then the follow-up question, do we need this?
There was a problem hiding this comment.
hmmm no we probably don't
…ve unnecessary synchronization
ahdinosaur
left a comment
There was a problem hiding this comment.
Looks good! If CI is happy I'll merge. ⛵
|
Thanks @DaneSlattery ! 🍰 |
|
Thank you! Love the library, and happy to contribute. |
Add a non-blocking LED-click event channel from the render thread to DesktopDriver, so desktop examples can consume selected LED indices via try_take_led_click() -> Option.