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

Add HD OSD support #3071

Merged
merged 2 commits into from
Nov 20, 2022
Merged

Add HD OSD support #3071

merged 2 commits into from
Nov 20, 2022

Conversation

SteveCEvans
Copy link
Member

Adds support for HD OSD, standardising on a 53x20 grid, utilising 24x36 fonts for 720P and 36x54 fonts for 1080P.

Grids for NTSC, PAL and HD are now supported.

image

And whilst moving items the grid is now a checkerboard.

image

This is for discussion. Coordinating setting of vcd_video_system and osd_displayport_device and improving the font (ideally colour to match the HD goggles in HD mode) are clear obvious enhancements.

See also betaflight/betaflight#11964.

@McGiverGim
Copy link
Member

I think I prefer the dashed background (like here #3008 and not the checked one. Other thoughts?

@SteveCEvans
Copy link
Member Author

It’s just an image (or rather a set of three). I think the old dashed one looked a bit clunky, hence this suggestion. Happy to use whatever image people find clearest.

@McGiverGim
Copy link
Member

The link I posted removes the background image and uses css to draw it.

@blckmn
Copy link
Member

blckmn commented Nov 5, 2022

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> PASS
  • 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 -> FAIL
  • assigned to an approver -> FAIL
  • approver count at least three -> FAIL

@SteveCEvans
Copy link
Member Author

@McGiverGim Yes, I appreciate that. Both PRs take the obvious step of defining VIDEO_COLS to define the screen size. The more significant change in this PR is to handle the 6 bit X encoding. I’m open to different CSS.

@McGiverGim
Copy link
Member

Please verify the unit tests. Is detecting position declared twice.

src/js/tabs/osd.js Outdated Show resolved Hide resolved
src/js/tabs/osd.js Outdated Show resolved Hide resolved
src/js/tabs/osd.js Outdated Show resolved Hide resolved
@SteveCEvans SteveCEvans force-pushed the hd_osd branch 2 times, most recently from 31dcecb to 552b1f6 Compare November 6, 2022 16:49
@github-actions

This comment has been minimized.

@benlumley
Copy link
Contributor

benlumley commented Nov 7, 2022

If anyone wants me to merge the CSS change + ditch the images as @McGiverGim is prefering; @me and i'll happily do it on top of this branch. Pretty sure it's an easy copy/paste mind!

And to add - my thinking for switching to CSS was to remove the requirement to create new images for any other new screen sizes that may come into being.

Ignore me.... i kept reading and see the above has already been done; and even better that variable grid sizes signalled by the video system have been added. Nice work!! 🥳

McGiverGim
McGiverGim previously approved these changes Nov 10, 2022
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Seems good to me!

@haslinghuis
Copy link
Member

haslinghuis commented Nov 10, 2022

Flashed the firmware as there is some issue after saving to HD on OSD tab and going to motors tab.

image

After several manual connection attempts it went into DFU mode (only once).
Something in d3 library is not happy. It's repeatable however sometimes need to repeat a few times to have the issue.

McGiverGim
McGiverGim previously approved these changes Nov 11, 2022
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Approved, but one question...

Comment on lines 1965 to 2080
if (videoType === 'AUTO') {
videoType = 'PAL';
}
Copy link
Member

Choose a reason for hiding this comment

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

One question. We have a way to know if there is an HD system configured/attached? I don't have the device, so I'm not sure if we have this information. In this case it will be interesting to select HD and not PAL here.

Copy link
Member Author

Choose a reason for hiding this comment

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

What I could do is change the firmware so that if a MSP_SET_OSD_CANVAS command is received, the vcd_video_system could be set to HD. If a save is done subsequent to that with the VTX connected, then both that and the canvas size would be preserved and used even if the VTX isn't powered.

See betaflight/betaflight@fa11425

Copy link
Member

Choose a reason for hiding this comment

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

Is not enough if MSP+Displayport is selected? Is simply a best effort, we let the user modify it if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

MSP VTX and displayport are one combined option and don’t necessarily mean HD.

@SteveCEvans SteveCEvans requested review from McGiverGim and haslinghuis and removed request for McGiverGim November 11, 2022 19:32
@haslinghuis
Copy link
Member

@SteveCEvans guess you didn't notice the back ticks here otherwise the ${var} template literal is not expanded

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

@github-actions

This comment has been minimized.

@haslinghuis haslinghuis requested a review from sugaarK November 15, 2022 02:00
src/js/tabs/osd.js Outdated Show resolved Hide resolved
@haslinghuis haslinghuis modified the milestones: 10.10.0, 10.9.0 Nov 15, 2022
@sugaarK
Copy link
Member

sugaarK commented Nov 15, 2022

Will review once changes made.. really happy this got bumped to 10.9 this is a big feature that will make the new release more exciting for the people

src/js/tabs/osd.js Outdated Show resolved Hide resolved
src/js/data_storage.js Outdated Show resolved Hide resolved
src/js/tabs/osd.js Outdated Show resolved Hide resolved
src/js/msp/MSPHelper.js Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@sugaarK
Copy link
Member

sugaarK commented Nov 20, 2022

@SteveCEvans what’s the B concuss on the @KarateBrot changes ?

@sonarcloud
Copy link

sonarcloud bot commented Nov 20, 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.0% 0.0% Duplication

@github-actions
Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@haslinghuis haslinghuis merged commit 266d5f4 into betaflight:master Nov 20, 2022
@McGiverGim McGiverGim mentioned this pull request Nov 30, 2022
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.

7 participants