[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Vbat precision increment #161

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Vbat precision increment #161

wants to merge 11 commits into from

Conversation

tylercorleone
Copy link
Member

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.

@gretel
Copy link
Contributor
gretel commented Apr 9, 2020

@tylercorleone thanks!

@gretel gretel added the enhancement Minor enhancement to code label Apr 9, 2020
@gretel
Copy link
Contributor
gretel commented Apr 10, 2020

it's working fine 🥇

@gretel gretel self-requested a review April 10, 2020 01:29
@gretel gretel requested a review from Quick-Flash April 10, 2020 01:29
Quick-Flash
Quick-Flash requested changes Apr 10, 2020
View reviewed changes
Copy link
Member
@Quick-Flash Quick-Flash left a 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!

@gretel
Copy link
Contributor
gretel commented Apr 10, 2020 •
edited
Loading

@Quick-Flash approvals are not intended to block merges, please stop that or do not review

@tylercorleone
Copy link
Member Author

It's a pleasure for me to contribute :)
guys, you decide when to merge, I enjoy making these changes, but I don't know the project's roadmap, so manage them as you see fit!

Quick-Flash
Quick-Flash approved these changes Apr 13, 2020
View reviewed changes
Copy link
Member
@Quick-Flash Quick-Flash left a 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.

@nerdCopter nerdCopter added the do-not-merge hold from merging until fixed or otherwise label Apr 13, 2020
@nerdCopter
Copy link
Member

i think the point was to not approve, nor request change.
i've created a label "On-Hold" and applied it. i'm open to renaming or not using, but it's an option.

@gretel
Copy link
Contributor
gretel commented Apr 13, 2020 •
edited
Loading

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.

gretel
gretel reviewed Apr 14, 2020
View reviewed changes
src/main/interface/msp.c
sbufWriteU8(dst, batteryConfig()->vbatmincellvoltage);
sbufWriteU8(dst, batteryConfig()->vbatmaxcellvoltage);
sbufWriteU8(dst, batteryConfig()->vbatwarningcellvoltage);
sbufWriteU8(dst, (batteryConfig()->vbatmincellvoltage + 5) / 10);
Copy link
Contributor

Choose a reason for hiding this comment

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

why changing the values?

Copy link
Member Author

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!

Copy link
Member Author

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

Copy link
Contributor

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

Copy link
Contributor
@loutwice loutwice Apr 15, 2020 •
edited
Loading

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

Copy link
Contributor

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 ;-)

Copy link
Member Author

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 tylercorleone mentioned this pull request Apr 15, 2020
Closed
@gretel
Copy link
Contributor
gretel commented Apr 15, 2020 •
edited
Loading

@tylercorleone wait i commented on the wrong pr.. branch :) i'm getting this set vbat_comp_ref = 84 issue using the code commited here.

@tylercorleone
Copy link
Member Author

@tylercorleone wait i commented on the wrong pr.. branch :) i'm getting this using the code commited here.

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_ref

vbat_comp_ref = 370
rateprofile 0
Allowed range: 100 - 500

@nerdCopter nerdCopter marked this pull request as draft May 9, 2020 15:02
nerdCopter added a commit that referenced this pull request May 28, 2020
@nerdCopter
bump version 0.2.26
43f4a3d 
* expert-mode show/hide toggle Angle,SPA,SDS,WC (#161)
* expert-mode LPF (#158)
* expert-mode show/hide toggle OSD (#159)
nerdCopter pushed a commit that referenced this pull request May 28, 2020
@Quick-Flash
Merge pull request #161 from nerdCopter/20200501_expert_SPA_SDS_WC_Angle
328b33c 
Expert-Mode show/hide toggle for Angle, SPA, SDS, WC
@gretel
Copy link
Contributor
gretel commented Jun 13, 2020

@tylercorleone it was flying this code for long now.. can you check if it still works fine one recent master? regards

@Quick-Flash
Copy link
Member

@nerdCopter if you can do the gui updates then we can get this merged.

@nerdCopter
Copy link
Member

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.

@Quick-Flash
Copy link
Member

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?

@nerdCopter
Copy link
Member

Any update here?

zero effort since.

@nerdCopter nerdCopter added the help wanted Extra attention is needed label Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gretel gretel gretel approved these changes

@loutwice loutwice loutwice left review comments

@Quick-Flash Quick-Flash Quick-Flash approved these changes

Assignees
No one assigned
Labels
do-not-merge hold from merging until fixed or otherwise enhancement Minor enhancement to code help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tylercorleone @gretel @nerdCopter @Quick-Flash @loutwice @fgiudice98