pymavlink
pymavlink copied to clipboard
ArduPilot
generate WIP warnings on the use of WIP messages in C code
now reversed it so it generates warnings only if you ask it too this also makes it more portable
@tridge
- So how do you expect this to be used? I think you are suggesting that a flight stack that wants to prevent WIP messages in their environment would add the following line to whatever code they have that includes the library?
#include <mavlink/mavlink.h>
#define MAVLINK_WIP __attribute__((warning("MAVLink work in progress")))
- FMI, I'd like to run the tests with this active - ie use
./test_generator.sh. How do I inject this into the test suite?
@hamishwillee yes, that's how it would be used
on (2), the problem is we run tests with -Werror, so any warnings would make the test fail
on (2), the problem is we run tests with -Werror, so any warnings would make the test fail
Yes, I want to run the tests, watch them fail, and see the message :-)
It was more FMI than anything else, because it isn't obvious (to me) how you could do this if you wanted given that most of the test is autogenerated.
If it were me I would additionally add a file level comment about the fact the message is WIP. My concern is users who see a message and try to use it without understanding the implications. You might not consider that a high risk.
I'm OK with this fix otherwise. I don't have any magic to merge or approve this.
When we use similar approach for deprecated tag, wouldn't it be better if that was "default on"?
@peterbarker @auturgy Should this be merged?
I don't love the implementation in that the whole idea was that WIP stuff should not be used in production. tridge has his reasons, but I doubt that people will think to enable this, which makes it pointless.
I would have preferred perhaps that the library was built without WIP messages by default and you had to specifically enable them using a build time flag. That of course would be a bit frustrating because it would be all or nothing - you want a WIP thing in ardupilot you have to take the ones in common too.
We are moving away from using WIP in common, but I suspect that dialects may still find it useful to keep WIP, and therefore possibly to have this warning.
Ideally we’d build with WIP by default in Master/Dev branches but not in production/release branches.
On 17 Nov 2021, at 4:17 pm, Hamish Willee @.***> wrote:
@peterbarker @auturgy Should this be merged?
I don't love the implementation in that the whole idea was that WIP stuff should not be used in production. tridge has his reasons, but I doubt that people will think to enable this, which makes it pointless.
I would have preferred perhaps that the library was built without WIP messages by default and you had to specifically enable them using a build time flag. That of course would be a bit frustrating because it would be all or nothing - you want a WIP thing in ardupilot you have to take the ones in common too.
We are moving away from using WIP in common, but I suspect that dialects may still find it useful to keep WIP, and therefore possibly to have this warning.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
Ideally we’d build with WIP by default in Master/Dev branches but not in production/release branches.
@auturgy Yes, though I guess that would have its own challenges when you cut the release and realize you actually rely on something WIP. Obviously we can do it, but not without development effort. What would be good is if we could agree how we would like this to work, so at least someone writing a generator can do the right thing here.
Is there any strong feeling on what you want ArduPilot side?
I also vote to remove WIP items from stable releases. What do you think @tridge?
A new PR needs to be created because after the https://github.com/ArduPilot/pymavlink/pull/592 merge, the WIP attribute needs to be added to the enumeration.
I think that elements with the WIP tag should be ignored by the parser (mavparse.py), not every generator.
@peterbarker @auturgy Should this be merged?
I think so.
I don't love the implementation in that the whole idea was that WIP stuff should not be used in production. tridge has his reasons, but I doubt that people will think to enable this, which makes it pointless.
I would have preferred perhaps that the library was built without WIP messages by default and you had to specifically enable them using a build time flag. That of course would be a bit frustrating because it would be all or nothing - you want a WIP thing in ardupilot you have to take the ones in common too.
I think that's acceptable.
We are moving away from using WIP in common, but I suspect that dialects may still find it useful to keep WIP, and therefore possibly to have this warning.
It's kind of neither here nor there. It's not a great look from a standards-perspective having things in common.xml which are WIP.... if we can move everyone to using all.xml then we can gradually move to only having them in development.xml.
I also vote to remove WIP items from stable releases. What do you think @tridge?
We need to discuss this in the context of the generated C mavlink headers here: https://github.com/mavlink/c_library_v2
If we were to add an option to mavgen.py to omit <wip/> tags - would that repository start to use that option? The implication there is that suddenly anything marked <wip/> would disappear from any project using those headers - so, for example, qgroundcontrol (https://github.com/mavlink/qgroundcontrol/blob/master/.gitmodules#L4)
That could be a lot of breakage in a short amount of time. This might be considered a transition issue? Should the pre-compiled headers come in two flavours, with-WIP and without?
Problem then becomes "what gets built with WIP, what doesn't?". So, for example, do QGC's daily builds ship with WIP messages and the releases not? ArduPilot's "latest" releases with-WIP and "stable" releases without?
One big advantage of not including WIP messages is that field definitions can change semantics during development without changing the checksum of the message. Might this lead to dangerous situations?
@peterbarker FWIW my leaning is not to remove WIP stuff from builds. Had we decided to build without the WIP stuff at the beginning it would have been good/ok. But I think you're right and it might create a lot of breaks now. It would certainly need to be tested and initially default as "not removed".
PX4 now does the same thing as ArduPilot and builds the libraries from sources. I don't know what QGC does. But either way, we'd be adverse to changing the generated libraries if we don't know for sure that we won't be breaking others.
Longer term the goal is to move all wip out of common, where at all possible: either things are in development.xml or they are actually common, or they are in a dialect. It is slow going.
Anyway, my leaning is that this PR now makes sense for dialects. Or we could just close it and move on. Nothing happened for 4 years and we didn't feel that it was a problem :-0
As discussed in dev call and https://github.com/mavlink/mavlink/pull/1924#issuecomment-1341651246
Our preference is that mavgen builds without WIP entities by default
- I am assuming enum values will always be "at the end" and so removing them will not screw up ordering.
- The entities could then be added back with a build flag and/or environment variable.
- CI would want to build both versions.
- We want warnings if APIs are used that are WIP. Above Tridge indicated this breaks CI. Can we make the warning behind a flag too - which can be disabled in testing.
@peterbarker We closed #1924 in December 2022 in favour of this PR added in 2018 on your suggestion. You're right - this is a better technical solution than constructing XML definition include files to omit development.xml. Or at least it is a better solution for C which is our library of most concern.
This has basically been blocked on the request to enable the warning by default. I think after 5 years we can agree that the Pymavlink dev team can block doing the "bleedingly obvious correct thing" forever right?
How about you update this to fix the merge conflicts and merge as is? That will be very annoying, and mean that that we will continue to have WIP leaking into releases. BUT it does mean that it is possible for developers to more easily get these warnings.
Thoughts?
Thanks very much @peterbarker .
What needs to happen to get this in? Do you need more testing, or something else? I can help.
Just a ping for @peterbarker and @tridge - is there anything we can do to get this in?
@hamishwillee we would need to remove the wip tags on the opendroneid messages first, or all builds that use -Werror (including ArduPilot) will fail
there is also the basic problem that we use -Werror in CI in ArduPilot. So as soon as a new WIP message gets added that we want to use in ArduPilot master (which is quite often) we would fail CI
@tridge Thank you. So can't you adjust CI to turn this default off in CI - default on everywhere else?
If this is not possible then as above, we'd rather get this in default off than have no way to check for WIP.
@hamishwillee we would need to remove the wip tags on the opendroneid messages first,
W.r.t. ODID none of those messages have WIP. There are WIP things though, such as https://mavlink.io/en/messages/common.html#MAV_CMD_DO_ORBIT - I'm happy to do another round of accepting/removing if that is needed.
So can't you adjust CI to turn this default off in CI - default on everywhere else?
we don't want it for developers doing local builds, or in the custom build server. The point is I don't want lots of warnings being shown to people who can't do anything about the warnings.