Drake: Create an LcmAndAbstractValueTranslator

[liangfok]

Motivation

Sometimes an LCM message cannot be translated into a drake::systems::VectorBase and vice versa. Instead, it can only be meaningfully translated into a drake::systems::AbstractValue.

Action Item

Create an LcmAndAbstractValueTranslator that is modeled after the existing drake::systems::lcm::LcmAndVectorBaseTranslator.

Additional Thoughts

It would be nice if LcmAndVectorBaseTranslator and the proposed LcmAndAbstractValueTranslator could share the same ancestor class so they can both be used by LcmPublisherSystem and LcmSubscriberSystem.

[jwnimmer-tri]

I can help talk though the design, if anyone wants. You don't technically need a common base class, though it's a perhaps still good choice.

[jwnimmer-tri]

My two very very rough design ideas:

(1) Can we share the same translator interface or base class or something, and have it offer either/or vector and abstract translations. (The abstract could by default try to downcast to a vector and delegate to a vector translator.)

(2) Or, could we have LCM publisher and subscriber classes offer overloaded constructor (and dictionary support for) either kind of translator -- and just pass in whichever kind you want to use.

Or in general, just try to share as much as possible between the abstract and vector sides of the fence.

I haven't worked through any of these designs, but would be happy to help with anyone who picks this up.

[tkoolen]

Should we first decide whether to also tackle cases such as https://github.com/RobotLocomotion/drake/issues/3360#issue-175312645?

Maybe we shouldn't try to do too much. On the other hand, it may help us come up with a better design. For example, to support cases such as https://github.com/RobotLocomotion/drake/issues/3360#issue-175312645, I think getting rid of LcmAndVectorBaseTranslator and simply having users inherit from LcmPublisherSystem and LcmSubscriberSystem might be a better design choice. That way, users can fully specify all of the input and output ports they want.

[jwnimmer-tri]

It's worth settling on whether LcmFooSystem should be a single-ported system always, or sometimes can be multi-ported. If single-ported, I think updating the Translator concept(s) to be what we need will be better in the long run, because it separates the concerns well IMO. If multi-ported, we'll need to revisit the Translator concept.

My gut feeling so far is that the base system code's contributions to #3360 are to move the values from the wire into the normative / minimal format in a Vector or AbstractValue; if the SpecificValue wants to (lazily or aggressively) compute derived values beyond that, I don't think it should be the Translator's problem.

[tkoolen]

What is SpecificValue above? I think I still understood what you were trying to say, but just to make sure.

Hey, how's this: we could have LcmSubscriberSystem not do any translation, instead giving it an abstract-valued output port that just spits out the untranslated but decoded LCM message (e.g. an object of type lcmt_driving_command_t). Then it's up to the user to connect that port to a System that either directly translates it into a VectorBase, or translates it into a different non-LCM object, or multiple non-LCM objects on separate ports. Same idea for LcmPublisherSystem. I think that would be much more flexible, and nicely separates out just the networking stuff.

[jwnimmer-tri]

What is SpecificValue above?

Ah, perhaps Value<Specific> would be better. I meant some concrete value, that isn't a vector.

Hey, how's this: we could have LcmSubscriberSystem not do any translation, instead giving it an abstract-valued output port that just spits out the untranslated but decoded LCM message...

That is neat, but I'm not sure how the Publisher and Subscriber learn how to do that. I am trying to keep knowledge of lcmt_foo_t out of those systems, so they don't need to be recompiled over and over again. What I think that means is to keep the Translator idea, but that it should convert between bytes and a specific message type, where the message object is just an AbstractValue as far as the System knows. (So, the Translator is essentially a lambda around the LCM-generated serialization methods, with no special System2 vector or application-level semantics; just bytes <-> AbstractValue.) That is elegant in terms of the Publisher / Subscriber systems, and clear in terms of the responsibilities of a Translator, and serves the AbstractValue use case (this ticket) well.

We'd have to see how well it matched up with downstream systems that really just want some basic continuous input vector (e.g., is there too much ceremony for DrivingCommandVector as an input to a system), but I suspect that could get solved pretty easily with some sugar as part of the rework.

[tkoolen]

(all comments below have analogues for LcmPublisherSystem)

I would template LcmSubscriberSystem on the LCM message type, but I understand from your comment above and from comments I saw on a previous pull request that you don't like that. I personally think not needing to have a std::vector<uint8_t> at an interface outweighs the added compilation time (which I think will be small).

But if you really don't like that, why not instead have LcmSubscriberSystem have an abstract-valued output port where the underlying type is a std::vector<uint8_t>?

Here's a reason I don't like having the translator be a part of LcmSubscriberSystem. Suppose you want to eventually split up the data in the LCM message into data presented on several different ports. Then you need to create an intermediate type to which the LCM message gets translated, whose only reason for existence is to be the input for the system that splits it up into the types you really want to use. Why not simply have the intermediate type be the LCM message itself (or its bytes)?

[liangfok]

How about let's keep the Translator idea since it nicely encapsulates all of LCM and instead generalize its inputs and outputs. Currently, the translator works with one BasicVector<double> on the System 2.0 side, and one LCM message on the LCM side. How about let's generalize it to work with a set of named BasicVector<double> and AbstractValue objects on the System 2.0 side and a set of named LCM messages on the LCM side? The challenge will then be to modify LcmPublisherSystem and LcmSubscriberSystem to do the right thing with these named inputs and outputs. Basically, I am thinking each named input and output will correspond to a particular System 2.0 input / output port or LCM publisher / subscriber.

[jwnimmer-tri]

I have what I think will be a good, unifying proposal. I'll need some time today to prototype it though.

[jwnimmer-tri]

Having worked it through a bit, I think having LcmSubscriberSystem offer an abstract-valued port with the decoded message object, and LcmPublisherSystem offer an abstract-valued port with the encoded message object, would meet all needs well. The byte vector can stay internal to the LcmFooSystem. Consumers who want the message mapped onto a single vector can start from the message object itself, then populate their BasicVector. I will post a WIP sketch of this later today.

[liangfok]

Just one port? I guess the original description of this issue only requries a single port, but maybe we should generalize to support multi-ported systems given #3360?

[tkoolen]

Sounds great to me. @liangfok, this supports that scenario quite well, I'll make a sketch and post it in a second.

[liangfok]

Oops, I guess I used the word "port" too loosely. I meant should we generalize the LCM systems to support producing and consuming more than one LCM message on more than one channel?

[tkoolen]

I don't think that's necessary. If you want to listen to more than one channel, just create multiple LcmSubscriberSystems, one for each channel. Same thing for publishing. Or is there some downside to that that I'm not seeing?

[liangfok]

I see, now that I read #3360 more closely, I see that you only need to combine data from multiple System 2.0 ports into a single LCM message. Let's table the idea of supporting multiple LCM messages within a single LCM{Publisher, Subscriber}System until a clear need arises.

[jwnimmer-tri]

https://github.com/jwnimmer-tri/drake/tree/lcm-abstract-message-port is very much not close to a design proposal yet, but it's as far as I could type before my meeting now. The idea is to have the Systems remain non-templated, but a type-erased SerializerInterface special for the different message types.

[jwnimmer-tri]

... and I think I can easily add enough sugar for it to be brief and trivial to use correctly.

[liangfok]

@jwnimmer-tri I was looking again at your WIP branch here: https://github.com/jwnimmer-tri/drake/commit/62c8e5fcbaecdb793b002147e6e039d05c111d4f. Is it true that in your new design, Lcm{Publisher, Subscriber}System will only use a single AbstractValue port? Does this mean we're going to need Mux / DeMux systems that combine multiple ports (e.g., joint position, joint velocity, joint force/torques, and sensor force/torque data as mentioned in #3360) into a single AbstractValue Port?

Also, since AbstractValue is not a parent class of BasicVector, are we going to need a system whose sole purpose is to extract the BasicVector vector or vectors out of an AbstractValue? Since we can't convert between AbstractValue and BasicVector using type erasure, I'm wondering whether it makes sense to remove native BasicVector support from the Lcm{Publisher, Subscriber}System systems.

[tkoolen]

Does this mean we're going to need Mux / DeMux systems that combine multiple ports (e.g., joint position, joint velocity, joint force/torques, and sensor force/torque data as mentioned in #3360) into a single AbstractValue Port

Keep in mind that the _LCM message type_ will be the underlying value of the AbstractValue port. At some point, in any design, several signals _have to_ be merged into this one type. The new design just obviates the need to create an additional intermediate non-LCM message type that the signals get merged into.

For #3360 (on the publisher side), there would be a System, say RobotStateEncoder, that translates all of the required signals coming out of the sensors attached to the RigidBodyPlant into an LCM message, which is presented on an output port that is hooked up to the LcmPublisherSystem.

(note again that robot_state_t is kind of a bad name for this message)

are we going to need a system whose sole purpose is to extract the BasicVector vector or vectors out of an AbstractValue

Yep. In the previous design, this was exactly the role of the LcmAndVectorBaseTranslators. The basic idea in this new design is to convert the existing translators into proper Systems that (on the subscriber side) take an LCM message AbstractValue and produce whatever outputs are needed. We should probably add some sugar that streamlines the special case of making a system that translates from one LCM message to one output, and vice versa.

By the way, I think it's much nicer to separate the Deserialize and Serialize parts of LcmAndVectorBaseTranslator into different Systems, because there will frequently be cases where you should really only need to implement one or the other; see e.g. ViewerDrawTranslator.

[liangfok]

OK sounds good. Thanks for the detailed explanation and figure! Since RobotStateEncoder creates an LCM message, I'm now wondering, once #3643 is merged, whether RobotStateEncoder should simply publish the message directly using a DrakeLcmInterface* rather than encapsulate the LCM message within an AbstractValue that is then sent to an LcmPublisherSystem. Will there be other consumers of the AbstractValue holding the LCM message?

[jwnimmer-tri]

@liangfok One good reason to have a Value<robot_state_t> output port is that it lets you use a transmission channel other than LcmPublisherSystem. LcmPublisherSystem is only a good idea for realtime-locked "simulations" with hardware (or a visualizer) in the loop. When you're actually in a closed-loop simulation (ignores wallclock), you might want to simulate the message transmission itself, and sometimes you'll want to do that with a Value<> and not the bytestream (both are valid choices).

[liangfok]

OK I buy that argument. Thanks.

I remain a little apprehensive of using an LCM message as the underlying object stored in the AbstractValue that RobotStateEncoder outputs. I feel doing that will overly couple Drake to LCM. In other words, that design will prevent people who select not to include LCM (like the minimal build) from using RobotStateEncoder. Can we compromise by having RobotStateEncoder output an AbstractValue that includes a RobotState object, which contains sufficient API sugar for users to understand and access the data within it, including the ability to construct an LCM message from the data?

[tkoolen]

In my opinion, from a usability standpoint, it's just annoying to force people to write the RobotState object that looks exactly like the LCM message but without the LCM includes, when all they want to do is publish robot_state_t.

Nothing is stopping people from replacing both the RobotStateEncoder and LcmPublisherSystem blocks with something else for a test without LCM.

[jwnimmer-tri]

@liangfok We should enable people who want LCM to use it as easily as possible, which this issue / PR should help with. Generalizing code for different transports is a yak-shave, and best done with more specific goals in mind.

[liangfok]

Yes, I agree. Note that in my previous post I didn't mention any alternative transports.

[jwnimmer-tri]

Good point.

I guess also I don't see the purpose of a general-purpose System that "encodes" robot state into an LCM-agnostic representation. If there's reusable code worth carving out, we can do that with C++ techniques, and not System blocks. System blocks are a very expensive abstraction, to be used only where systems concerns come into play. For software reuse, C++ should be the first tool, and System blocks a distant second.

[liangfok]

OK. We'll use C++ approaches to reuse RobotStateEncoder when or if the need arises.

[tkoolen]

@jwnimmer-tri and @liangfok, can I take this over from you?

[liangfok]

Yes of course. I am not working on this at the moment.

[jwnimmer-tri]

I'm of two minds on this. My WIP branch is pretty far along, but I was going to try to convert everything at once (all current users of these two systems) to prove the new design and code work well

However, it's possible I could PR some intermediate working code to unblock your work @tkoolen? Do you need pub, sub, or both? I could possibly get a smaller PR out very soon.

[tkoolen]

I'm even fine just working off of your branch as long as the basic functionality is there and won't change too much anymore. A quick PR would be ideal though.

pub, sub, or both

Ideally, both pub an sub, but if you only have time for one, I would prefer sub; I know you're busy.

[jwnimmer-tri]

Sanity check...

LcmPublisherSystem and LcmSubscriberSystem currently operate on a vector-valued port. Per this issue, we also want to support abstract-valued ports, where the message object comes in / out (as a Value<lcmt_drake_foo>), instead of a vector-mapping of the message object.

I think the easiest way to transition this, and possibly even the best long-term design, is to have the class offer a constructor-time choice of whether you want abstract-valued or vector-valued input / output, but still have just the one port. Does that sound reasonable?

(Alternatively, you could imagine that the Subscriber system might offer _both_ ports for downstream to use, or that we _only_ support Value<lcmt...> ports, and the translator-into-continuous-vector is an entirely separate System.)

If we like that idea, the shape of the PR would be:

1. Adding a factory method to each of those two systems to request abstract input / output (caller supplies the templated message type),
2. Having a bit more complicated member data (to track which kind of ports we wanted) and System method overrides (to shape and populate the ports correctly),
3. Adding a bunch more test coverage.

I think a second PR could follow that refactored the LcmAndVectorBaseTranslator concept to use message objects instead of byte vectors, but I think I can skip that in the first PR.

The WIP branch I posted was doing "we _only_ support Value<lcmt...> ports", but I think it would be faster to retrench to the "either / or" ports and get that merged, and possibly that's best overall anyway.

Thoughts?

[jwnimmer-tri]

Well, I did it in my branch and I'm pretty happy with it:

• https://github.com/jwnimmer-tri/drake/commits/lcm-abstract-message-port

It lacks some comment tidying (and possibly other tidying), and lacks unit testing. I hope it is a good checkpoint for this work.

Realistically, it's likely next Tuesday or later before I have this substantially further. @liangfok or @tkoolen feel free to take this as a start and see if it meets your needs. If so, you are welcome to polish and PR it, or just post feedback or patches, or wait until next week and I'll try to wrap it up.

[liangfok]

From a quick scan of your WIP, I believe it'll serve our purpose. I notice numerous if (translator) { foo } else { bar } code fragments. It makes me wonder whether there should be a common parent interface and two child classes one implementing each branch of the if statement.

[jwnimmer-tri]

From a quick scan of your WIP, I believe it'll serve our purpose. I notice numerous if (translator) { foo } else { bar } code fragments.

That would be improved by the _"I think a second PR could [refactor] the LcmAndVectorBaseTranslator concept to use message objects instead of byte vectors, but I think I can skip that in the first PR."_.

[liangfok]

Great! Let's keep it two PRs.

[tkoolen]

_only_ support Value

That was the way I was envisioning this. Also, or instead, having a Vector output based on constructor arguments seems a lot less elegant to me (witnessed by the if (translator) statements). But I'm happy to run with this for now. Thanks!

If you make further progress, could you stop squashing/amending for now?

[jwnimmer-tri]

If you make further progress, could you stop squashing/amending for now?

Wilco.