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 new symbols to max7456_symbols.h #8138

Merged
merged 2 commits into from
May 4, 2019
Merged

Conversation

Docteh
Copy link
Contributor

@Docteh Docteh commented May 1, 2019

Goes together with betaflight/betaflight-configurator#1392

Some changes for new fonts
Fixes the crosshair
Also changed GPS LAT and LON elements to use new symbols
MPH and KPH symbols used

Fixes the crosshair
Also changed GPS LAT and LON elements to use new symbols
McGiverGim
McGiverGim previously approved these changes May 1, 2019
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.

Thanks for that!

@@ -25,6 +25,14 @@
#define SYM_END_OF_FONT 0xFF
#define SYM_BLANK 0x20
#define SYM_COLON 0x2D
#define SYM_BBLOG 0x10
Copy link
Member

Choose a reason for hiding this comment

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

One rule we have is that we generally only define symbols that are actually used - this makes it easy to sort out what slots are still available in a font.

So my recommendation is to comment out the defines for symbols that were added but aren't yet used, and uncomment them in the pull request that puts them to use.

mikeller
mikeller previously approved these changes May 1, 2019
@Docteh
Copy link
Contributor Author

Docteh commented May 1, 2019

Uhoh I broke a unittest.

time to learn about unit tests.

Also turns out the Core Temperature widget is subjected to unit tests
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

Good stuff on fixing the tests!

@mikeller
Copy link
Member

mikeller commented May 2, 2019

The question is, do we want to merge this and betaflight/betaflight-configurator#1392 now, or do you want to go full monty and do a pull request to change the OSD preview in configurator to use the new symbols as well, and then we merge all 3 together?

@McGiverGim
Copy link
Member

I did it here: betaflight/betaflight-configurator#1413

After this first batch is merged, we need to use some of the new symbols as prefix for actual elements, but I think is better to let it to a later PR.

@mikeller mikeller merged commit 9554415 into betaflight:master May 4, 2019
@Jetrell
Copy link

Jetrell commented May 5, 2019

I gave it a run. And it looks great in action.

The only character that must have slipped by was 0x7f. It is the Altitude prefix symbol.

@mikeller
Copy link
Member

mikeller commented May 5, 2019

@Jetrell: There's #8184 and betaflight/betaflight-configurator#1420...

@Jetrell
Copy link

Jetrell commented May 5, 2019

@mikeller I say this reluctantly. Because I don't want to put more load on you and the guys, than you already have.
But with respect to -
Capture
Home_flag and RPM. The code is there for both of these symbols as well. i.e. Distance to home and ESC RPM telemetry.
What I mean is. Now that the extra characters for these functions are merged. The odds of the original writers of those pieces of code, going back and upgrading the symbols, are slim. Because they may not be aware that there exist.

I say this in the nicest possible way.... It would appear that the character symbols have come second to the code itself, over the years. Which is fine. But I feel that now would be a good time to clean it all up, while the vacuum cleaner is out.... I know that's easy for me to say.... when I'm not doing the cleaning... If I was able I would!

Does that make scenes ?

@mikeller
Copy link
Member

mikeller commented May 5, 2019

@Jetrell: All good - this is a mess, as is often the case with community driven open source projects, especially when lots of moving parts are involved (firmware, configurator, font files...).
We'll clean up at some point. :-)

@McGiverGim
Copy link
Member

Yes, you can open an issue when you find things that need to be done. In this way when someone has some time will do it. Is not a problem to notice it 😉

@Docteh Docteh deleted the 41x_symbols branch May 10, 2019 04:14
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.

None yet

4 participants