Add a frame_id field to LocationFix

[defunctzombie]

The frame_id field for the LocationFix message specifies the frame for the origin of the sensor so the lat/lon values can be tied to a location on the robot.

Fixes: #100

---

Nothing in Studio uses this information yet but we can introduce this for folks that want to start recording it.

[snosenzo]

Wondering if this is the right way to add this? I think this might need a headerstamp to address a frameId at a specific header time as well, no?

Is there a reason we're doing it this way and not adding a new TransformWithLocationFix message?
Just trying to think about what usage would look like

[defunctzombie]

I think this might need a headerstamp to address a frameId at a specific header time as well, no?

Yea - we need to add a timestamp field like we have with other messages as well.

Is there a reason we're doing it this way and not adding a new TransformWithLocationFix message?

What would this message be?

To me the reason to go with this approach is that it uses the same pattern that other sensor data uses which is that there is a frame_id for the sensor and some message data tied to that frame.

[snosenzo]

To me the reason to go with this approach is that it uses the same pattern that other sensor data uses which is that there is a frame_id for the sensor and some message data tied to that frame.

Ah yeah, that makes sense. I guess defining a new kind of transform message would make it harder to access this, so nevermind on that.

Yeah, so all this needs really is the timestamp.

[defunctzombie]

I've added a timestamp.

[amacneil]

nit: every other schema has timestamp listed before frame_id

[amacneil]

I know that it doesn't affect us directly due to mcap storing schemas in the file, but it seems against the spirit of protobuf to change these field identifiers right? It would break backwards compatibility for anyone trying to use new schemas to read old data?

Not sure if we have already made breaking changes like this before?

[amacneil]

If we wanted to solve this while still allowing the fields to appear in desired order (timestamp at the top), we might need to introduce some explicit protobuf field ID into schemas.ts to manually assign these. I think it's only an issue for protobuf.

[defunctzombie]

@jtbandes Have we done anything like this yet?

@amacneil I think we roughly treat these schemas as a snapshot set of examples rather than "use these exactly for all possible use-cases you have". Maybe we should call out this nuance in protobuf more explicitly but I've never known this repo to have a goal of keeping things backwards compatible.

[jtbandes]

Yeah we have, search for protobufFieldNumber

[amacneil]

I think we roughly treat these schemas as a snapshot set of examples rather than "use these exactly for all possible use-cases you have". Maybe we should call out this nuance in protobuf more explicitly but I've never known this repo to have a goal of keeping things backwards compatible.

Yeah agreed I don't think we explicitly make this promise. But it does seem a bit weird from a user's perspective for us to unnecessarily ship backwards-incompatible changes to the schema. So maybe we should make that promise going forward unless there is a particular change where that would be impossible. In this case looks like we just need to explicitly specify the numbers.

[defunctzombie]

Thanks for the tip on protobufFieldNumber I've added that to LocationFix to keep the existing field numbering unchanged.

[snosenzo]

We should probably make a ticket for testing that we don't break protobuf field numbers when updating the schema if there isn't one already