Skip to content

Add OSD_TOTAL_PACKS (with betaflight/pull/10808) #2532

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
Closed
wants to merge 1 commit into from
Closed

Add OSD_TOTAL_PACKS (with betaflight/pull/10808) #2532

wants to merge 1 commit into from

Conversation

@bree29
Copy link

@bree29 bree29 commented Jul 9, 2021 •
edited by haslinghuis
Loading

Add OSD_TOTAL_PACKS

Satisfies Needs coordination with betaflight-configurator label on the original PR #10808.

I tested the betaflight-configurator on Linux64 platform, and with modified Betaflight 4.3 (the one in PR) on STM32F7X2 target

# config: manufacturer_id: AIKO, board_name: AIKONF7, version: 34f72565, date: 2019-10-03T17:08:47Z

All settings went good on BF configurator and seems OK.

I'll test the feature itself this weekend ; all is looking good on the betaflight-configurator side.

@bree29 bree29 changed the title Add OSD_TOTAL_PACK (with betaflight/pull/10808) Add OSD_TOTAL_PACKS (with betaflight/pull/10808) Jul 9, 2021
@asizon asizon added this to the 10.8.0 milestone Jul 9, 2021
asizon
asizon requested changes Jul 9, 2021
View reviewed changes
src/js/tabs/osd.js Outdated
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_42)) {
OSD.constants.STATISTIC_FIELDS = OSD.constants.STATISTIC_FIELDS.concat([
F.STAT_TOTAL_FLIGHTS,
F.STAT_TOTAL_PACKS,
Copy link
Member

@asizon asizon Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It should be added to API_VERSION_1_44.

asizon
asizon requested changes Jul 9, 2021
View reviewed changes
src/js/tabs/osd.js Outdated
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
OSD.constants.DISPLAY_FIELDS = OSD.constants.DISPLAY_FIELDS.concat([
F.TOTAL_FLIGHTS,
F.TOTAL_PACKS,
Copy link
Member

@asizon asizon Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also this bit should go the last one on the list, the order is important here.

@bree29 bree29 closed this Jul 9, 2021
@bree29 bree29 reopened this Jul 9, 2021
@bree29 bree29 closed this Jul 9, 2021
@bree29 bree29 reopened this Jul 9, 2021
@bree29 bree29 force-pushed the stats_total_packs branch from 6fae571 to ded6144 Compare July 12, 2021 19:42
@bree29
Copy link
Author

bree29 commented Jul 12, 2021 •
edited
Loading 

yarn
yarn test
yarn start

Feature tested on BF side (see original PR)

Reviews required by @asizon have been made.

@blckmn not sure why GitHub identifies PR as not mergeable. Any insight for that ?

Screenshot from 2021-07-14 12-34-31

@bree29 bree29 requested a review from asizon July 12, 2021 22:46
haslinghuis
haslinghuis previously approved these changes Jul 13, 2021
View reviewed changes
asizon
asizon previously approved these changes Jul 14, 2021
View reviewed changes
limonspb
limonspb reviewed Aug 1, 2021
View reviewed changes
locales/en/messages.json Outdated
"description": "One of the statistics that can be shown at the end of the flight in the OSD"
},
"osdDescStatTotalPacks": {
"message": "Total number of used packs"
Copy link
Member

@limonspb limonspb Aug 1, 2021

Choose a reason for hiding this comment

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

flown packs could be better? Need a native English speaker here :)

@bree29 bree29 closed this Aug 2, 2021
@bree29 bree29 force-pushed the stats_total_packs branch from ded6144 to 2273b36 Compare August 2, 2021 10:14
@bree29 bree29 reopened this Aug 5, 2021
@bree29 bree29 dismissed stale reviews from asizon and haslinghuis via 2558e29 August 5, 2021 14:02
@bree29
Copy link
Author

bree29 commented Aug 5, 2021 •
edited
Loading

Accidently closed the PR alongside my rebase. Everything should work great right now. Please let me now.
Edit : damn. What now. I see rebase's changes in commit.

@bree29 bree29 force-pushed the stats_total_packs branch from 2558e29 to bac67a4 Compare August 5, 2021 14:07
haslinghuis
haslinghuis previously approved these changes Aug 13, 2021
View reviewed changes
@bree29 bree29 requested review from asizon and limonspb August 21, 2021 11:42
asizon
asizon previously approved these changes Aug 22, 2021
View reviewed changes
Copy link
Member

@asizon asizon left a comment

Choose a reason for hiding this comment

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

LGTM!

@asizon asizon added the Tested label Aug 22, 2021
@asizon asizon self-assigned this Aug 22, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 28, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

mikeller
mikeller previously approved these changes Aug 31, 2021
View reviewed changes
@ctzsnooze
Copy link
Member

ctzsnooze commented Dec 4, 2021

@bree29 - If you want this merged into 10.8, please squash your commits and indicate that you still think it is important.

Otherwise we will tag it for 10.9 and think about it after 4.3 / 10.8 are released.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 4, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

@haslinghuis haslinghuis modified the milestones: 10.8.0, 10.8.1 Dec 5, 2021
@haslinghuis haslinghuis modified the milestones: 10.8.1, 10.9.0 Jan 10, 2022
@bree29 bree29 dismissed stale reviews from mikeller, ctzsnooze, asizon, and haslinghuis via fa4bbad January 10, 2022 21:59
@bree29 bree29 force-pushed the stats_total_packs branch from c035192 to fa4bbad Compare January 10, 2022 21:59
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 10, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@bree29
Copy link
Author

bree29 commented Jan 10, 2022

@bree29 - If you want this merged into 10.8, please squash your commits and indicate that you still think it is important.

Otherwise we will tag it for 10.9 and think about it after 4.3 / 10.8 are released.

Just in case my updates from upstream will be an issue (the "one commit" thing, that were 5 here), just as it was in the betaflight part, I followed the same solution to update it to HEAD & squash my commit. Since I must do something wrong in my rebase sequences.

Sorry for the inconvenience of having to re-approving it for merging.

@haslinghuis
Copy link
Member

haslinghuis commented Jan 10, 2022

Merging upstream changes does count as commit but is actually not when rebasing. Found out after using letting vscode to do this for me. So I'm back on the command line.

@blckmn
Copy link
Member

blckmn commented Jan 11, 2022

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> FAIL
  • approver count at least three -> FAIL

@asizon
Copy link
Member

asizon commented Jan 11, 2022

Firmware pr is labeled for 4.4,i think we are not in time to merge this new feature into 4.3

@bree29
Copy link
Author

bree29 commented Jun 18, 2022

@limonspb , do I need to rebase before reviews are approved ? Same question for the firmware PR

@haslinghuis
Copy link
Member

haslinghuis commented Jun 18, 2022

@bree29 there are no conflicts. Both PR's are ready to merge. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@limonspb limonspb limonspb approved these changes

@haslinghuis haslinghuis haslinghuis approved these changes

@mikeller mikeller mikeller left review comments

@ctzsnooze ctzsnooze ctzsnooze left review comments

@asizon asizon asizon left review comments

Assignees

@bree29 bree29

Labels

Needs coordination with firmware Rebase Required Rejected Feature Request RN: IMPROVEMENT Tested

Projects

None yet

Milestone

10.9.0

Development

Successfully merging this pull request may close these issues.

7 participants

@bree29 @haslinghuis @ctzsnooze @blckmn @asizon @limonspb @mikeller