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

Drop attempts to set MIS_TOTAL parameter #11482

Merged
merged 5 commits into from
Aug 21, 2019

Conversation

peterbarker
Copy link
Contributor

Closes #11413

@peterbarker
Copy link
Contributor Author

We discussed this at a devcall. There are more elaborate solutions which would fix the problem more nicely, but this works in the short term.

Copy link
Contributor

@WickedShell WickedShell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not thrilled about the solution, as this is messy from a GCS side as there isn't an ACK and it just looks like packet loss from the protocol.

Tools/autotest/common.py Show resolved Hide resolved
@peterbarker
Copy link
Contributor Author

peterbarker commented Jun 4, 2019 via email

@peterbarker
Copy link
Contributor Author

This has been modified to notify the GCS that we accepted their set - but instantly send the current value. This is so we don't break the expected parameter state machine.

STABILIZE> param set MIS_TOTAL 17
STABILIZE> APM: Param write denied (MIS_TOTAL)
< PARAM_VALUE {param_id : MIS_TOTAL, param_value : 17.0, param_type : 4, param_count : 1139, param_index : 65535}
< PARAM_VALUE {param_id : MIS_TOTAL, param_value : 3.0, param_type : 4, param_count : 1139, param_index : 65535}

param set RC4_TRIM  1600
STABILIZE> < PARAM_VALUE {param_id : RC4_TRIM, param_value : 1600.0, param_type : 4, param_count : 1139, param_index : 65535}

@tridge
Copy link
Contributor

tridge commented Jun 10, 2019

better to use a flag bit in param table

@peterbarker
Copy link
Contributor Author

Reworked to use a new parameter bit indicting a parameter is for internal use only.

@peterbarker
Copy link
Contributor Author

.... so am I going back to the string-matching thing on this one?

OXINARF
OXINARF previously requested changes Aug 2, 2019
Copy link
Member

@OXINARF OXINARF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One improvement we should have in the future is allowing the top parameters to have flags too.

libraries/GCS_MAVLink/GCS_Param.cpp Outdated Show resolved Hide resolved
libraries/AP_Param/AP_Param.h Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Show resolved Hide resolved
@peterbarker peterbarker dismissed OXINARF’s stale review August 20, 2019 00:53

Points have been addressed - thanks!

@peterbarker
Copy link
Contributor Author

@tridge @WickedShell As discussed/requested on the DevCall, I've fixed this up based on @OXINARF 's review - should be good to go now.

Copy link
Member

@OXINARF OXINARF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@peterbarker peterbarker merged commit 2049e86 into ArduPilot:master Aug 21, 2019
@peterbarker peterbarker deleted the pr/no-set-mission-total branch August 21, 2019 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copter: AUTO with no mission items causes crash
6 participants