Support reserved/default params in generated libraries #269
Conversation
|
I'm not a huge fan of this. Because these aren't really messages there is no way to enforce this at a library level, which means that NaN's are still easily not emitted and still require the implementor of sending the message to insert the NaN's correctly. The attribute makes sense on a top level message, but on something that is encoded into COMMAND_LONG, COMMAND_INT, MISSION_ITEM, MISSION_ITEM_INT among others I think there are to many ways for that to not reflect the default of NaN. |
|
@WickedShell I am convinced this is a good idea for exactly the reason you are not. Because there is no way to enforce this in (all) the API(s) the best thing you can do is add the documentation at point-of-use (in generated library) - to increase the chance that the end user will see the info and "do the right thing". We could make it more clear that this is something the user has to "do" . In addition (separate from the NaN issue), this ensures that the in-source documentation is actually usable by adding visible index. Currently if someone omits a param definition it is impossible to tell what definition applies to what param in the docs). So even if we were to remove the "default value" from this, I'd still say this is real improvement. Edited (PS) The docs have been updated to state that the 5,6 params should not be NaN. I'd love to enforce this via the validator, but I guess there are cases where you might create a command that should only be used with the all-float-field-type messages. |
|
@peterbarker @tridge Can I please get this reviewed? I spoke to @WickedShell and he is conceptually OK with this - his review did not understand that this change is about in-source docs more than anything else. |
|
|
||
| #Post process to add reserved params (for docs) | ||
| for current_enum in self.enum: | ||
| if not 'MAV_CMD' in current_enum.name: |
There was a problem hiding this comment.
pythonically if 'MAV_CMD' not in current_enum.name:
| for current_enum in self.enum: | ||
| if not 'MAV_CMD' in current_enum.name: | ||
| continue | ||
| print(current_enum.name) |
There was a problem hiding this comment.
There's a lot of debug still in here.
|
|
||
| for a_param in enum_entry.param: | ||
| params_dict[int(a_param.index)] = a_param | ||
| enum_entry.param=params_dict.values() |
There was a problem hiding this comment.
Should be a space in here.
Generally it would be nice if flake8 didn't whinge about new stuff.
|
Merged, thanks! |
XML definition files can now declare command params as reserved and define a default value of 0 or NaN. If a param is not declared it is assumed to be reserved with default of zero.
What this PR does is implement that behaviour and generate improved docs:
paramvalue descriptions. This makes it much easier to work out what text maps to which param, and makes it clear that you're in a param even if it has no description.The change is in the stored description in the parser, so will flow to all outputs.
It doesn't prevent someone declaring param 6 or 5 as reserved values with NaN default value (which is invalid when sending in COMMAND_INT). My understanding is that the schema should prevent that in validation, but the parser should respect XML if validation is not run (?)