-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
Vbat precision increment #161
base: master
Are you sure you want to change the base?
Vbat precision increment #161
Conversation
@tylercorleone thanks! |
it's working fine 🥇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, i'm going to "request changes" not because this is wrong or bad, I just want to wait until after 0.3.0 to merge this into the code (simply because this messes with the gui a little and would require a gui update). But it looks good. Thanks!
@Quick-Flash approvals are not intended to block merges, please stop that or do not review |
It's a pleasure for me to contribute :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested by @gretel I will approve this, However, this should not be merged until after the next release.
i think the point was to not approve, nor request change. |
the root cause is the configurator being broken and not getting maintained. there is no actual relation but some people trying to manage "user experience" in a strange way. flight code is flight code and this is not the configurator repository. |
sbufWriteU8(dst, batteryConfig()->vbatmincellvoltage); | ||
sbufWriteU8(dst, batteryConfig()->vbatmaxcellvoltage); | ||
sbufWriteU8(dst, batteryConfig()->vbatwarningcellvoltage); | ||
sbufWriteU8(dst, (batteryConfig()->vbatmincellvoltage + 5) / 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why changing the values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because these tree are the legacy values (uint8_t), while vbatmincellvoltage is an uint16_t so we have to transform a 370 in 37, for example. This way a legacy receiver will receive the correct value.
Below we write the values with full precision!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an idea of mine, I took from the cherry-pick :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i get it know thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't touch msp please without adding numlber in api version
it breaks all the stuff for the gui ... you can change everything in the code execpt the msp
@tylercorleone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the root cause is the configurator being broken and not getting maintained. there is no actual relation but some people trying to manage "user experience" in a strange way. flight code is flight code and this is not the configurator repository.
besides, this project should have had a roadmap ages ago. so people can actually agree on something instead of acting in their own preference.
well msp is not flight code ... it is the contract between gui and firmware it have to be done as a road map as you said ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loutwice I have to increment API_VERSION_MINOR in this case? Or it's better to rollback the changes to the MSP?
@tylercorleone wait i commented on the wrong pr.. branch :) i'm getting this |
Hi @gretel I just tried a fresh build from this branch, flashing with full erase and I obtain this (no diffs uploaded): get vbat_comp_refvbat_comp_ref = 370 |
Expert-Mode show/hide toggle for Angle, SPA, SDS, WC
@tylercorleone it was flying this code for long now.. can you check if it still works fine one recent |
@nerdCopter if you can do the gui updates then we can get this merged. |
yeah, i've created a local branch days ago and tried to merge this (massive conflicts). i need a big chunk of time to get this + gui up to par. |
Any update here? |
zero effort since. |
This PR fundamentally contains the cherry-pick of this PR: https://github.com/betaflight/betaflight/pull/7255/commits plus a couple of changes needed because of conflicts.