Removing ownership of menu context

[ryan-summers]

This PR fixes #17 by updating the Context to no longer be owned by the Runner. Instead, an interface is owned (to handle I/O) and the context is borrowed to the runner when IO is being processed.

[thejpster]

Ah, clever.

Will this be a breaking change for existing menus?

[ryan-summers]

Indeed this is a very breaking change and can be annoying for the end user to update because all of the APIs for callbacks change as a result. I've updated the example and added a CHANGELOG for this.

I'm wondering if now also makes sense to update to using the embedded-io traits instead of core::fmt::Write and input_byte as well, but honestly I don't mind the current approach anyways.

[jordens]

Could the owned interface be dropped again if the borrowed context supports Write?
Fewer type parameters...

[ryan-summers]

Could the owned interface be dropped again if the borrowed context supports Write? Fewer type parameters...

The interface is needed even in cases where the Context is not available, such as Runner::prompt, so if we didn't have the borrowed context in this case, we wouldn't be able to generate output data

[ryan-summers]

That being said, We could update this so that the interface and Context are combined together (as in the existing master implementation), but just borrow context in all cases, and thus we'd need to change the API of prompt to mutably borrow the Context as well. That would keep the type signatures the same

[thejpster]

If the Context supported embedded-io, you wouldn't need to pass bytes - you'd just call menu.process(&mut context) and it would use the context to read them.

[ryan-summers]

I suspect this may be the cleanest implementation. Let me take a look at what that would look like. Thanks for the feedback!

[jordens]

In our case we'd probably construct a throw-away Context for each process() call. It's not exactly beautiful, but might be leaner overall.

[ryan-summers]

No, that also wouldn't work because the Context would have to have a concrete type (i.e. it would need to know the lifetime of the borrowed data).

In our case, we would have to move the ownership of our Context (i.e. Stabilizer's Settings) into our interface (i.e. Stabilizer's Platform`). However, I don't think this is too big of an issue

[thejpster]

If you have a diff for a project that uses this crate, that would need very interesting to see

[ryan-summers]

@thejpster Check out https://github.com/quartiq/stabilizer/pull/872/files#diff-f0b20facc8d3d2dfccda7199e70cb8af5ba9255d80e1d35fdd950dc6a2d567a3 - we are using menu in a separate crate serial-settings that serves as a generic serial interface to configure device settings, and it has a static menu. This is also the driver that made me realize the borrowed context issue as well

[thejpster]

Looks good to me. Do you want to merge this as-is, add look at those other changes later?

[ryan-summers]

Let me take a look at what the refactor would be tomorrow and how it looks for the end user :) I suspect it could simplify things

[thejpster]

I like this - just a few notes.

[thejpster]

Is this still true now context is passed out of band?

[ryan-summers]

Yes this is still true because of borrowing self.buffer for the string and then mutably borrowing self when calling self.prompt()

[thejpster]

ahh

[thejpster]

but can prompt be made non-mut?

[ryan-summers]

While this newer approach is nicer internally to menu, it can be a somewhat awkward user experience, as the borrowed context may now have multiple layers of indirection to get down to the real context (i.e. to differentiate between I/O and data). See quartiq/stabilizer#874 for an example, where there's a number of wrapper structs to get at the internal Settings values

[thejpster]

I'm not sure why it's different in that regard to how it was. The main benefit here seems to be that the caller can own the context and the runner separately, and bring them together when there are characters to process.

[ryan-summers]

It's not different w.r.t the released version of menu, but it is somewhat different w.r.t 7ba8849. Specifically, the issue is as follows:

• In our case (stabilizer), Context is really a combination of two things: the USB interface for chracter I/O and an internal Settings tree. The Settings tree is used in various drivers (to facilitate distributing device settings across the firmware).
• In our case, the desired Context is an RTIC resource that needs to be locked to access, i.e. it is a Mutex<Context>. We want to lock the mutex to achieve the borrow and then provide the locked value to menu for processing.

What this means is that we now have to wrap our Settings structure and USB interface into a single RTIC resource so that it can be singularly locked when menu::process() is called. This is just a decent amount of overhead within our code base, since anything that needs just the Settings structure now has to lock both the USB and Settings items and then grab at the inner Settings. This isn't a deal-breaker, it just adds a fair amount of boilerplate and indirection to our code base, and I wonder if this would be true in other cases.

---

The root of the issue ultimately is the anonymous lifetime borrow. Whatever the Context type is has to be a singular resource (since otherwise the borrow lifetime can't be specified as a concrete type).

[thejpster]

So you'd prefer two objects: one interface object and one context object? Could you use a tuple of them both as a single context?

[ryan-summers]

So you'd prefer two objects: one interface object and one context object?

I'm not sure if this is an issue that other users will face or one that arises out of our firmware architecture honestly, so I'm not sure if I know the "right" way.

Could you use a tuple of them both as a single context?

This is the root of the problem. This would make the context have a type of (Interface, &'a Settings). However, the 'a lifetime is still unknown.

---

My gut reaction is that it might make more sense to have separate Interface and Context items, but I'm not 100% sure that's the right design for everyone.

[thejpster]

But if you pass a value of that type to the Runner's input _bytes method, who needs to know the type?

[ryan-summers]

The problem is that we have the menu::Runner object stored as an RTIC resource, so the generic parameter passed in as the context needs to be concrete.

[ryan-summers]

@thejpster In all honesty, I don't know what the right path is. Given that, I'd say we leave this as-implemented in this PR and move forward and see how it goes. We can always come back to it later.

If things look good to you, feel free to re-approve/merge as appropriate :) Thanks for the review!

[jordens]

@ryan-summers Didn't we have the the impression that combining interface and context was cumbersome for applications, and that 7ba8849 (plus picking the e-io updates) was better (despite the two type parameters)?

[ryan-summers]

@ryan-summers Didn't we have the the impression that combining interface and context was cumbersome for applications, and that 7ba8849 (plus picking the e-io updates) was better (despite the two type parameters)?

Yes, but I'm not convinced that's true for the general use case of this library or not. Two type parameters is more flexible, but it does make the library slightly more complex to use as well. I'm somewhat ambivalent as to what the right approach is personally given that our only test case is currently stabilizer.

[jordens]

I now regret having floated the idea ;)
Given what it looks like for all our applications, (stabilizer, fls, booster, thermostat, thermostat-eem, ...) I'd want the two independent types.

[ryan-summers]

The more I think about it, the more @jordens is convincing me that we should revert to 7ba8849, specifically because that the Interface can be owned by the runner without issue, but the Context shouldn't be owned (to allow borrowing).

If we combine Interface and Context, it requires that we logically combine two items that may have nothing to do with one another (i.e. Settings have nothing to do with the USB interface). This is what causes the weird ramifications on end user firmware.

[thejpster]

Should the runner own the interface or should it have a poll(&mut intf, &mut ctx) method?

[ryan-summers]

Should the runner own the interface or should it have a poll(&mut intf, &mut ctx) method?

My opinion here is that the interface is much more closely tied to menu than the context. The context, by definition, is intended be a method to move data between menu and other system components. In contrast, the interface is more an integral part of menu because it provides the means of processing input and output to the menu itself.

I would imagine that it is rarer for the user to want to use the interface with more things than just menu. And in cases where that's required, it's possible to create synchronization primitives to facilitate this that live outside of menu.

I'm not opposed to the idea, but I would recommend we do that in potentially a follow-on PR instead

[thejpster]

Thank you for the changes and the thoughtful discussion! It is greatly appreciated.