message_definitions: add all_with_development.xml #1924
Conversation
With this commit we have: - all_with_development.xml: for development and message ID conflict checking. - all.xml: for released software connecting to all flight stacks, e.g. QGC, MAVSDK, pymavlink.
|
@julianoes Don't we also need to build both of these in CI? |
|
We're looking at including "all.xml" in QGC as this means that we automatically get all stakeholder dialects supported. The argument against that was that we don't want development.xml in releases, because then we end up with the problems we had using "WIP" in common - stuff we might want to modify that we no longer can. So all.xml now splits out development. If you really want to build with development then you use "all_with_development.xml".
@bkueng As discussed offline, I understand you want to be able to build QGC with development.xml. Can you do what is needful to ensure that releases do not ever include development, but daily builds do ("all_with_development.xml")? Note that having a build flag or whatever is OK but it should be set up so that the default is all.xml and you have to do something special to get the development version. My reasoning here is that PX4 v1.13 released with development because that was the default setting in main when the release was cut. |
|
On Wed, 23 Nov 2022, Hamish Willee wrote:
We're looking at including "all.xml" in QGC as this means that we automatically get all stakeholder dialects supported. The argument against that was that we don't want development.xml in releases, because
then we end up with the problems we had using "WIP" in common - stuff we might want to modify that we no longer can.
I think this bears some thought. By *not* building with development.xml
we seriously limit exposure and possible testing of new messages. This is
really only relevant for modified older code-paths that would be triggered
by the new messages rather than GCS features based around a new message.
Particularly in the (relatively!) resource-unconstrained environment a GCS
has, having a checkbox which allows acceptance of "WIP" messages
should be a real option.
So all.xml now splits out development. If you really want to build with development then you use "all_with_development.xml".
That breaks stated semantics for all.xml - there might already be some
GCSs and/or tools out there that are relying on the current semantics, and
I'd prefer not to break them. We can not break by creating a
"all_without_development.xml". all.xml then includes both
"all_without_development.xml" and "development.xml". That's really rather
an ugly name, but doing things this way won't break existing users.
* Is this conceptually and practically OK
* Is there an other places that need to be updated - e.g. tests
tests would need to be updated - we *really* do want to ensure that we
don't double-allocate IDs.
@bkueng As discussed offline, I understand you want to be able to build QGC with development.xml. Can you do what is needful to ensure that releases do not ever include development, but daily builds do
("all_with_development.xml")? Note that having a build flag or whatever is OK but it should be set up so that the default is all.xml and you have to do something special to get the development version. My
reasoning here is that PX4 v1.13 released with development because that was the default setting in main when the release was cut.
Having that checkbox in QGC to allow or disallow use of WIP messages would
simplify this rather a lot.
I do realise that checkbox wouldn't easily *stop* you sending a
WIP-message - perhaps someone can think of a good place to hook such a
check?
|
|
@peterbarker Thanks. Understood that we want to build all dialects so tests need to be updated.
Yes, the name is horrible, and I hate the fact that "all" doen't really mean "all". But sometimes you have to be pragmatic. If we want to use all.xml in ground stations and also not include development.xml in releases then this is probalby the best way. You might also consider that if we have users that have release code with development.xml, present via all.xml, we want to break them. They are doing the wrong thing using development in release code.
We want to discourage use of development.xml in releases. Some approach like this might help QGC achieve the result, but then the next system that comes along won't do the same thing. Anyway, let's give @bkueng and @julianoes a chance to comment. Definitely does require thought. |
|
Thanks for looking into this. I can see this go either way, build-time or run-time. |
Mmmmm.... depends on the implementation language. If we goofed with MAVProxy, for example, we could release a version of the code depending on development messages - and that wouldn't be caught until someone crossed the relevant codepath. I'm not all that familiar with MissionPlanner development, but I believe it might be possible to get the same effect in there - runtime-loading of code is great, but also the pits. I think on balance Hamish is right, 'though. I'll ask for a bit of time at our DevCall tomorrow to discuss. |
|
why not make a qgc-release.xml that just leaves-out development.xml? |
|
WIP tags were an early attempt to manage experimental behaviour. We moved to development.xml as a cleaner way of managing what is "standard" - in part because we never got ability to exclude WIP tags by default. We're working to move WIP out of common.xml - either accepted or moved to development, or something else. We will occasionally need them though, because they are the only way currently we can extend some existing entities in common.
Still not sure what's best. Ambivalent as long as it is hard to include development.xml in releases. |
the same experimental behavior you want to keep out of QGC? combined with
it really sounds like excluding WIP tags is the correct thing to do. I actually thought we did have this facility and used it in the generated mavlink headers; can you point me to the failed effort, please? |
Yes "in release builds"
Yes, except that stuff in development.xml is assumed WIP but not necessarily tagged as such. But yes, we could do something to default mark everything as WIP or we could explicitly do so in development.xml.
This would fail because it needs to be enabled - no one will remember to do this. It might work if it was default "do not build" unless flag set. |
|
On Wed, 30 Nov 2022, Hamish Willee wrote:
the same experimental behavior you want to keep out of QGC?
Yes "in release builds"
it really sounds like excluding WIP tags is the correct thing to do.
Yes, except that stuff in development.xml is assumed WIP but not necessarily tagged as such.
We can change the one message that's currently in there :-)
... or remove it, since we've decided against it anyway...
I actually thought we did have this facility and used it in the generated mavlink headers; can you point me to the failed effort, please?
ArduPilot/pymavlink#240
This would fail because it needs to be enabled - no one will remember to do this.
Sorry, what would need to be enabled? The purging of WIP messages? Only
a handful of people will ever need to get that right :-) In the immediate
case QGC could start to filter out the WIP messages. If that would cause
unacceptable breakage then we've got an inconsistency and should remove
WIP from the messages causing issues.
If we get all the major players compiling against all.xml then we should
probably consider moving all WIP messages from common.xml to
development.xml for consistency's sake. Contrast with
OBSTACLE_DISTANCE_3D which ArduPilot hasn't made the efforts to make a
standardised message and is simply WIP in ardupilotmega.xml.
|
Yes. It's not so much a matter of purging messages as making sure they can easily be used during development and not during releases. It is far easier IMO to have an XML file that doesn't use them than to try have a GCS filter them after the fact. I'm more inclined to have a structure as suggested by Buzz. Interested in what @auturgy thinks.
I'm slowing working on that. Some of them are actually incorrectly marked. At the end of the process there should only be a few WIP items that have to be there. i.e. because they affect existing enums or similar that are defined in common. |
Because I'd also want to use it in MAVSDK, so I wouldn't make it specific to an implementation. |
I mean't "gcs-release". But you could also do the same thing with "all_dialects" |
|
On Tue, 6 Dec 2022, Hamish Willee wrote:
why not make a qgc-release.xml that just leaves-out development.xml?
.. and maybe a qgc-dev.xml that includes development.xml ?
Because I'd also want to use it in MAVSDK, so I wouldn't make it specific to an implementation.
I mean't "gcs-release". But you could also do the same thing with "all_dialects"
all > all_dialects_with_dev > all_dialects > dialects + common +standard+ minimal
> test
> others
I might not have been clear in what I was getting at.
I'd really rather not have any magic in the file names when there's a
technical measure (the WIP tag) available in all files to inform GCSs
use of experimental messages.
|
I'd agree if there was any help from the toolchain, but there is not. Essentially to use the WIP tag for this purpose we would have to fix the toolchain so that it will build the libraries without WIP entities, and we might also need to pre-build libraries both in release and development variants. That's possible, but needs work. A GCS that builds from sources might in theory capture additional information from the WIP tag to dynamically determine which methods to enable/disable at runtime. Again though, that's not supported and a lot of work. A GCS using pre-built libraries can't do this as the data is already lost. So right now, the easiest way to achieve this is to have a way to not build said messages in the first place. It's not pretty but it is pragmatic. If you are happy to add support for building libraries without WIP entities we can talk further about this option. Unless I'm missing something? |
|
We talked about this in the mavlink call last night. The broad agreement was that @peterbarker is right; using the WIP tags to filter messages is a better approach. Our preference is that mavgen builds without WIP entities by default
@peterbarker That means we need to re-invigorate, and probably update, ArduPilot/pymavlink#240. As I recall that warns on use of WIP tags, but you have to explicitly enable it. We think actually removing the entities is better, and prefer "default off". @peterbarker @julianoes @auturgy Closing this now. |
With this commit we have: