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

sensors: prefix mag config param SENS instead of CAL #20723

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Conversation

bresch
Copy link
Member

@bresch bresch commented Dec 7, 2022

Solved Problem

CAL_MAG_SIDES and CAL_MAG_ROT_AUTO are not calibration calibrations values of the mag sensor, they are config parameters and shouldn't be saved as part of a factory calibration.

Solution

Change the prefix to SENS_

@bresch bresch added the Sensors All the sensors! label Dec 7, 2022
@bresch bresch requested a review from dagar December 7, 2022 15:40
@bresch bresch self-assigned this Dec 7, 2022
@junwoo091400
Copy link
Contributor

This PR would get affected by this change. Could we get that PR in first & then rebase these changes on top? @bresch

#20247

@dagar
Copy link
Member

dagar commented Dec 8, 2022

Somewhat unfortunately QGC accesses CAL_MAG_SIDES directly.

https://github.com/search?q=repo%3Amavlink%2Fqgroundcontrol%20CAL_MAG_SIDES&type=code

@bresch
Copy link
Member Author

bresch commented Dec 8, 2022

Somewhat unfortunately QGC accesses CAL_MAG_SIDES directly.

if it reads it only, I can leave it but mark it as deprecated and force it to be following the value of SENS_MAG_SIDES directly in the code.

@cmic0
Copy link
Contributor

cmic0 commented Dec 8, 2022

@bresch something else to consider: this parameter was also stored in the EEPROM, by changing the name we now resolving the problem for future EEPROM parameter save, but previous vehicles that had this CAL_MAG_SIDES parameter saved on the EEPROM will keep loading it.
I think this will result in an annoying CAL_MAG_SIDES not found message, no?

@bresch
Copy link
Member Author

bresch commented Dec 8, 2022

I think this will result in an annoying CAL_MAG_SIDES not found message, no?

I re-added the CAL_MAG_SIDES parameter mainly because QGC needs it to display the correct UI so the parameter is still there and will be overwritten by SENS_MAG_SIDES. However I added a mavlink message to tell the user to use the new parameter in case he wants to change the new one and this message will probably show up when the calibration parameters are loaded.

@dakejahl
Copy link
Contributor

Why not change the logic for param filtering? I understand this is very concise and visually pleasing, but we could easily add a black list of parameters in addition to the CAL_* pattern matching. Then you aren't relying on parameter names, which could easily break in the future if someone unknowingly adds a new CAL_BLAH parameter. Just my 2 cents. Also seems like we might want to rename CAL_MAG_COMP_TYP as well.
https://github.com/PX4/PX4-Autopilot/blob/main/src/modules/commander/factory_calibration_storage.cpp#L50

@bresch
Copy link
Member Author

bresch commented Dec 13, 2022

Why not change the logic for param filtering?

Yes, that was the first idea, but since we anyway wanted to rename it for quite some time, I think it's a good opportunity. That said, I should maybe still add it to the filter now that I had to put it back for QGC

@@ -53,6 +53,15 @@
* @value 63 Six side calibration
* @group Sensors
*/
PARAM_DEFINE_INT32(SENS_MAG_SIDES, 63);
Copy link
Member

Choose a reason for hiding this comment

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

This could be bits now.

Copy link
Member Author

Choose a reason for hiding this comment

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

The user would then need to select manually which side he wants and I already see ppl not selecting the right ones and having calibration issues...

*
* Use SENS_MAG_SIDES instead
*
* @group Sensors
Copy link
Member

Choose a reason for hiding this comment

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

We could hide it in a different category?

Suggested change
* @group Sensors
* @group Sensors
* @category Developer

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. #20919

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

Successfully merging this pull request may close these issues.

None yet

5 participants