Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FENCE_ACTION update for per-fence actions #1629

Merged
merged 2 commits into from
May 6, 2021

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented May 6, 2021

This restores the FENCE_ACTION changes that were added in #1482 and reverted in #1626. These ones did not need to be reverted but were part of a bigger commit that did. I plan to add back the other changes too, but only when I have worked out how to do so safely.

@hamishwillee
Copy link
Collaborator Author

Self merging. Tests pass and enum changes previously approved.

@hamishwillee hamishwillee merged commit 3bcab35 into master May 6, 2021
@hamishwillee hamishwillee deleted the restore_fenceaction branch May 6, 2021 02:11
@olliw42
Copy link
Contributor

olliw42 commented May 6, 2021

@hamishwillee
you've seen my comment to #1626?
The short story: if you want to solve that "safely" you need to do a substantial change to mavgen (it was btw the final nail in the coffin for me to abandon mavgen)
This issue exists with mavgen since kind of ever but only is an issue if one is trying to generate messages for multiple dialects at the same time, which means essentially only for c_library_vX.
The "solution" to this issue was since kind of ever to cleverly arrange the sequence of dialect generations in update_c_library.sh, it's not guaranteed to work out for all possible include trees, but appeared to have always worked out in the past, and I believe would also now.
What I'm trying to say, if you want a "safe" solution you may have to do more work than you may think now, and the pragmatic and quick and dirty solution of the past might gain some appeal to you.
:)

@hamishwillee
Copy link
Collaborator Author

Thanks @olliw42 - I did! Yes, I am looking at options now. Yes I can "fix" this by building development.xml first, but I'm going to see whether we can do something more robust.

First part of that is ArduPilot/pymavlink#537. I think this is better either way.

The other problem looks like a bug. Essentially the generator is supposed to merge enum values that occur in dialects, and at each level you'll have that enum inside a #ifndef HAVE_ENUM_xxxx / #define HAVE_ENUM_MAV_CMD guard so you only get one definition, and it is the one for the level at which you chose to include.
But what is happening is that only the top level dialect enum definition is being created. The lower level definitions are simply omitted. So if you build development after standard, then development gets all the MAV_CMD values and common gets overwritten with an empty values. I'll have a look at that next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants