Field name clash in some MAVLink messages

[pietrodn]

In mavgen_python.py, all generated MAVLink message types have two class attributes, id and name, expressing the MAVLink message type.

However, these class attributes clash with the MAVLink messages with fields called id and name!
Indeed:

1. LOG_ENTRY, LOG_REQUEST_DATA, LOG_DATA, DISTANCE_SENSOR, BATTERY_STATUS, COLLISION have an id field;
2. DEBUG_VECT, NAMED_VALUE_FLOAT, NAMED_VALUE_INT, UAVCAN_NODE_INFO, DEBUG_FLOAT_ARRAY have a name field.

This name clash can cause problems in code depending on the semantics of the name and id attribute. Example:

>>> from pymavlink.dialects.v20.common import MAVLink_log_entry_message
>>> MAVLink_log_entry_message.name
'LOG_ENTRY'
>>> MAVLink_log_entry_message.id
118
>>> MAVLink_log_entry_message(id=70, num_logs=2, last_log_num=2, time_utc=1000, size=1024).id
70

[hamishwillee]

This also appears to affect Java as well - see https://groups.google.com/g/mavlink/c/9cRgGSRzCLw/m/x9TDaLAiCAAJ

@peterbarker For the java case (at least) they solved the problem by modifying the generated code for the id case. Is it worth thinking about doing this automatically for a couple of key field names - like id and name?

[peterbarker]

@hamishwillee I think ideally we remove .id and .name as attributes - unless they're actual attributes of hte message.

Sadly, that's a significant change and likely to break a lot of stuff. I believe we upped and broke the API for Java - that's less an option for the python bindings given their widespread use.

We could probably start towards moving people off using .id and .name and whatever message metadata we have - I think Python probably gives us the flexibility to do so.

The message ID should always we available with get_msgId - so its really a matter of weaning people off the use of .id. That might be one by specifying @attr on the default attribute values and warning anybody using them we're going to break them in a few years. There is no equivalent for name, however.

BTW, the problem is getting worse over time:
https://github.com/ArduPilot/pymavlink/blob/master/generator/mavgen_python.py#L359

where we move the data to I don't know. Fundamentally you can't use class attributes to store information about the message and also use the fieldnames as attributes on instances of the class. I expect we'll choose to just prepend underscores where we aren't storing the data elsewhere), so e.g. msg._lengths. Alternatively we could provide a function to retrieve an object which has all of the metadata about the message....

While this is all rather messy - but its been this way for a very long time :-)

[hamishwillee]

@peterbarker Thanks very much. The only thing I know for sure is that we should either fix this or explicitly state that we're not going to; not just leave it as an open issue.

Prepending underscores for the general attributes of the message like id makes sense to me. But as there doesn't appear to be any non-breaking solution I'm more concerned about agreeing a deprecation plan. Will be lead by you.

Can you talk to @tridge about what his preferred solution might be?

[peterbarker]

On Wed, 13 Jan 2021, Hamish Willee wrote: @peterbarker Thanks very much. The only thing I know for sure is that we should either fix this or explicitly state that we're not going to; not just leave it as an open issue.

I'm not sure that's always the case. Sometimes something isn't worth attacking until you hit some other major related problem, and tips it over the edge to "has to be done now".... In this case we should at least work out how we'll do it when it comes to it.

[tridge]

I think we should rename the fields to have an _ prefix

[hamishwillee]

OK. So @peterbarker I'll keep an eye on this to see if you decide to fix this and what the roll out/mitigation plan is.

[peterbarker]

On Tue, 19 Jan 2021, Hamish Willee wrote: OK. So @peterbarker I'll keep an eye on this to see if you decide to fix this and what the roll out/mitigation plan is.

@amilcarlucas put his hand up to make these changes. I expect there are going to be some wails of anguish as we up-and-kill a whole bunch of semi-broken things.....

[hamishwillee]

Great. @amilcarlucas Any movement on this?

[amilcarlucas]

No, not yet. Basically I need to do:

s/\.id/\._id/g
s/\.name/\._name/g
s/\.instance/\._instance/g

but I have not done it yet :(

[amilcarlucas]

Nope :( not that simple.
\me lowers his hand now.