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

process_sensor_caldata.py: regularly weighted over temperature thermal fitting #14091

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

NicolasM0
Copy link
Contributor

I opened a thread here to discuss about an issue and propose a fix: https://discuss.px4.io/t/issue-with-thermal-calibration-fitting/14582

Describe problem solved by this pull request
If the temperature ramp is not constant during heat up, the fitting give more weight to temperature with lower change rate. If heating become very slow (typically at the high temperature), the calculated polynomial can not fit at all where the temperature change rate was high (typically at low temperature)

Describe your solution
This commit add a re sample before fitting to have a more regular weight over temperature

@dagar
Copy link
Member

dagar commented Apr 30, 2020

For those that didn't click through to discuss initially (like me).

Screenshot from 2020-04-30 10-40-58

Screenshot from 2020-04-30 10-41-48

@stale
Copy link

stale bot commented Jul 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jul 29, 2020
@bresch
Copy link
Member

bresch commented Aug 3, 2020

I like the idea and we shouldn't drop it. What's blocking this?

@NicolasM0
Copy link
Contributor Author

I began to implement the onboard version but let it paused because I was in a rush on other tasks. I can try to finish it at the end of the month

@dagar
Copy link
Member

dagar commented Aug 5, 2020

I began to implement the onboard version but let it paused because I was in a rush on other tasks. I can try to finish it at the end of the month

How about bringing it in with an option to output the parameters based on the resampled data? Then by default it's in sync with the onboard calibration method, but we can still view the impact of resampling, and opt into the change trivially.

@dagar dagar self-requested a review August 5, 2020 16:26
priseborough
priseborough previously approved these changes Aug 5, 2020
@priseborough
Copy link
Contributor

priseborough commented Aug 5, 2020

We should get this in and handle the online version in a separate PR. In the interim, a note could be added to the documentation https://docs.px4.io/master/en/advanced_config/sensor_thermal_calibration.html to indicate the importance of maintaining a rate of temperature increase throughout the calibration range when using the internal method.

@dagar
Copy link
Member

dagar commented Aug 6, 2020

@NicolasM0 can you rebase or merge with master to resolve the conflicts?

@NicolasM0
Copy link
Contributor Author

This commit have been rebased but it is still a draft.
Do you want me to clean it (remove previous fitting), add an option for the new fitting or let it as it is ?

@dagar
Copy link
Member

dagar commented Aug 6, 2020

This commit have been rebased but it is still a draft.
Do you want me to clean it (remove previous fitting), add an option for the new fitting or let it as it is ?

If there are no objections and it's documented I'm fine with it.

@dagar
Copy link
Member

dagar commented Aug 6, 2020

If it's not too much trouble then simply adding an option to skip the resampling might be worthwhile for anyone considering using online calibration in production and wanting to sanity check the results with offline (more than visually).

the option '--no_resample' allows to disable resampling and have the
previous behavior
@NicolasM0 NicolasM0 marked this pull request as ready for review August 7, 2020 14:14
@dagar dagar merged commit 1dec1e6 into PX4:master Aug 28, 2020
@NicolasM0 NicolasM0 deleted the thermal_calibration branch January 27, 2021 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants