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 stick overlays to OSD #7167

Merged
merged 1 commit into from Dec 10, 2018

Conversation

Projects
None yet
3 participants
@pkruger
Copy link
Contributor

pkruger commented Dec 3, 2018

No description provided.

#ifdef USE_OSD_STICK_OVERLAY
{ "osd_stick_overlay_left", VAR_UINT16 | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_LEFT]) },
{ "osd_stick_overlay_right", VAR_UINT16 | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_RIGHT]) },
{ "osd_horizontal_char", VAR_UINT8 | MASTER_VALUE, .config.minmax = { 0, 255 }, PG_OSD_CONFIG, offsetof(osdConfig_t, overlay_horizontal_char) },

This comment has been minimized.

@mikeller

mikeller Dec 3, 2018

Member

If we go for this solution, we should go for fixed characters. If users like different styles, they can use different fonts, or they can edit their font.

This comment has been minimized.

@pkruger

pkruger Dec 3, 2018

Author Contributor

@mikeller
Tricky one. The default characters -, | and + used for the crosses have gaps between them, which looks a little blocky. (I have added a videos to the issue #3988). Users can edit them specifically, but then it messes up other places where they are used, like the startup screen of Betaflight. (to enter the OSD menu). Having them configurable enables users to either use the defaults as is or to change characters in their fonts that they never use, like the compass characters or GPS satellite characters for example.

This comment has been minimized.

@ledvinap

ledvinap Dec 3, 2018

Contributor

It may be better to use single array variable. Using different font is quite complicated, so user should be experienced enough to use it.

This comment has been minimized.

@mikeller

mikeller Dec 3, 2018

Member

I still think we should not try to address this by introducing parameters for the characters to be used. How about this: We hardcode on '-', '|', and '+' for now, and start working on adding better glyphs to the fonts (there are unused characters available). Once we have the updated fonts published in configurator, we'll change the defines in the firmware for the glyphs to be used to use the new custom made ones?

This comment has been minimized.

@pkruger

pkruger Dec 5, 2018

Author Contributor

@mikeller Makes sense. If we make them configurable, most people might never change them anyway. Ok. I'll remove the configurable characters and hard code them to '-', '|', and '+'.

@@ -31,6 +31,7 @@
#include <string.h>
#include <ctype.h>
#include <math.h>
#include <drivers/max7456.h>

This comment has been minimized.

@mikeller

mikeller Dec 3, 2018

Member

This should go into the section with the other drivers/ headers.

This comment has been minimized.

@pkruger

pkruger Dec 3, 2018

Author Contributor

Done.

This comment has been minimized.

@pkruger

pkruger Dec 3, 2018

Author Contributor

Removed, no longer required.

static void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos)
{

for (uint16_t x=0; x<OSD_STICK_OVERLAY_WIDTH; x++) {

This comment has been minimized.

@mikeller

mikeller Dec 3, 2018

Member

Please use space around operators, except on the inside of brackets.

This comment has been minimized.

@pkruger

pkruger Dec 3, 2018

Author Contributor

Done.

@@ -1198,6 +1265,15 @@ static void osdDrawElements(void)
osdDrawSingleElement(OSD_LOG_STATUS);
}
#endif

#ifdef USE_OSD_STICK_OVERLAY
if ((VISIBLE(osdConfig()->item_pos[OSD_STICK_OVERLAY_LEFT])) &&

This comment has been minimized.

@mikeller

mikeller Dec 3, 2018

Member

Hmm, this is a bit odd, and unintuitive - if we define separate item_pos items for the individual stick overlays, then we should also enable them individually.

This comment has been minimized.

@pkruger

pkruger Dec 3, 2018

Author Contributor

I guess that makes sense. I assumed they will always be used together, but there may be exceptions. Fixed.

This comment has been minimized.

@mikeller

mikeller Dec 4, 2018

Member

It's not so much that I expect users to use the OSD with only one enabled. But the way all other OSD elements work is 'user enables element => element shows'. Deviating from this behaviour here for no actual benefit will just be confusing for users if they (for whatever reason) enable an element and it does not show up.

@@ -1236,6 +1312,14 @@ void pgResetFn_osdConfig(osdConfig_t *osdConfig)
osdConfig->timers[OSD_TIMER_1] = OSD_TIMER(OSD_TIMER_SRC_ON, OSD_TIMER_PREC_SECOND, 10);
osdConfig->timers[OSD_TIMER_2] = OSD_TIMER(OSD_TIMER_SRC_TOTAL_ARMED, OSD_TIMER_PREC_SECOND, 10);

osdConfig->item_pos[OSD_STICK_OVERLAY_LEFT] = OSD_POS(1,(max7456GetRowsCount()-OSD_STICK_OVERLAY_HEIGHT));

This comment has been minimized.

@mikeller

mikeller Dec 3, 2018

Member

OSD isn't limited to MAX7456 devices, so max7456GetRowsCount() is wrong here.

This comment has been minimized.

@pkruger

pkruger Dec 3, 2018

Author Contributor

I have removed this.

@@ -101,6 +105,8 @@ typedef enum {
OSD_FLIP_ARROW,
OSD_LINK_QUALITY,
OSD_TOTAL_DIST,
OSD_STICK_OVERLAY_LEFT,
OSD_STICK_OVERLAY_RIGHT,

This comment has been minimized.

@mikeller

mikeller Dec 3, 2018

Member

We need to increment the version of the osdConfig parameter group here, as we are increasing OSD_ITEM_COUNT. See https://github.com/betaflight/betaflight/blob/master/docs/development/ParameterGroups.md

This comment has been minimized.

@pkruger

pkruger Dec 3, 2018

Author Contributor

Oops. Thanks, now fixed.

@mikeller mikeller added this to the 4.0 milestone Dec 3, 2018

@pkruger pkruger force-pushed the pkruger:3988-feature-add-sticks-verlay-to-in-flight-OSD branch 2 times, most recently from ede35f3 to 7b00452 Dec 3, 2018

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Dec 3, 2018

Updated with all the changes from the review above - thanks.

#ifdef USE_OSD_STICK_OVERLAY
{ "osd_stick_overlay_left", VAR_UINT16 | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_LEFT]) },
{ "osd_stick_overlay_right", VAR_UINT16 | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_RIGHT]) },
{ "osd_horizontal_char", VAR_UINT8 | MASTER_VALUE, .config.minmax = { 0, 255 }, PG_OSD_CONFIG, offsetof(osdConfig_t, overlay_horizontal_char) },

This comment has been minimized.

@ledvinap

ledvinap Dec 3, 2018

Contributor

It may be better to use single array variable. Using different font is quite complicated, so user should be experienced enough to use it.

@@ -1146,6 +1146,53 @@ static bool osdDrawSingleElement(uint8_t item)
return true;
}

#ifdef USE_OSD_STICK_OVERLAY
static void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos)

This comment has been minimized.

@ledvinap

ledvinap Dec 3, 2018

Contributor

Stick?

This comment has been minimized.

@pkruger

pkruger Dec 4, 2018

Author Contributor

Oops. Fixed. Thanks.

This comment has been minimized.

@pkruger

pkruger Dec 4, 2018

Author Contributor

Not so sure about the array variable, then the user would have to know which index is which parameter. With distinct names it seems easier.

This comment has been minimized.

@ledvinap

ledvinap Dec 4, 2018

Contributor

You can define enum for characters ... osdConfig()->overlay[osdStickHorizontal]

This comment has been minimized.

@pkruger

pkruger Dec 5, 2018

Author Contributor

Also, the characters will no longer be configurable as discussed above, so fewer parameters now.

static void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos)
{

for (uint8_t x = 0; x < OSD_STICK_OVERLAY_WIDTH; x++) {

This comment has been minimized.

@ledvinap

ledvinap Dec 3, 2018

Contributor

unsigned x = 0

This comment has been minimized.

@pkruger

pkruger Dec 4, 2018

Author Contributor

What do you mean here?

This comment has been minimized.

@ledvinap

ledvinap Dec 4, 2018

Contributor

Using non-native types on ARM si suboptimal (gcc emits uint8_t -> unsigned on almost every access).
(my opinion, not generally shared between BF programmers): You want unsigned variable, but size is not important. Using uint8_t looks like you want to limit variable size for some reason. Using uint32_t (which is equivalent to unsigned on target processor) looks like there is some reason to use exactly 32 bits. It is confusing when reviewing code (you have to think about it a little bit).
I prefer to stick to signed (sign importance is stressed), int - just a number and unsigned - unsigned number is enough (usually to avoid signed/unsigned warning with sizeof etc.)

This comment has been minimized.

@pkruger

pkruger Dec 5, 2018

Author Contributor

Interesting, that's correct. Just unsigned by itself is not very portable, but I guess the days of 16-bit processors are over. I'll change it to unsigned x. The same goes for void osdDrawStockOverlayAxis(uint8_t xpos, uint8_t ypos), to void osdDrawStockOverlayAxis(unsigned xpos, unsigned ypos) then? Not quite...

Show resolved Hide resolved src/main/io/osd.c Outdated
Show resolved Hide resolved src/main/io/osd.c
#ifdef USE_OSD_STICK_OVERLAY
if (VISIBLE(osdConfig()->item_pos[OSD_STICK_OVERLAY_LEFT])) {
osdDrawStickOverlayAxisItem(OSD_STICK_OVERLAY_LEFT);
osdDrawStickOverlayCursor(OSD_STICK_OVERLAY_LEFT, THROTTLE, YAW);

This comment has been minimized.

@ledvinap

ledvinap Dec 3, 2018

Contributor

(this works only in mode 2)

This comment has been minimized.

@pkruger

pkruger Dec 4, 2018

Author Contributor

Now updated to support both mode 1,2,3 and 4.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 5, 2018

Fixes #3988.

@pkruger pkruger force-pushed the pkruger:3988-feature-add-sticks-verlay-to-in-flight-OSD branch from 7b00452 to 111391f Dec 6, 2018

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Dec 6, 2018

Changes added as requested above as well as support for radio modes 1, 2, 3 and 4.

@@ -103,6 +103,11 @@
#define VIDEO_BUFFER_CHARS_PAL 480
#define FULL_CIRCLE 360

#define STICK_OVERLAY_HORIZONTAL_CHAR 0x2D

This comment has been minimized.

@ledvinap

ledvinap Dec 6, 2018

Contributor

It may be cleaner to use character constants (+,-,|,0)

This comment has been minimized.

@pkruger

pkruger Dec 8, 2018

Author Contributor

Done.

@@ -143,7 +148,27 @@ typedef struct statistic_s {
uint8_t min_link_quality;
} statistic_t;

typedef struct radioControls_s {
rc_alias_e left_vertical;

This comment has been minimized.

@ledvinap

ledvinap Dec 6, 2018

Contributor

(uint8_t may be used to save some space, enum in 32bit)

This comment has been minimized.

@pkruger

pkruger Dec 8, 2018

Author Contributor

Done.

}

uint8_t x_pos = (uint8_t)scaleRange(constrain(rcData[horizontal_channel], PWM_RANGE_MIN, PWM_RANGE_MAX), PWM_RANGE_MIN, PWM_RANGE_MAX, 0, OSD_STICK_OVERLAY_WIDTH);
uint8_t y_pos = (uint8_t)scaleRange(PWM_RANGE_MAX - constrain(rcData[vertical_channel], PWM_RANGE_MIN, PWM_RANGE_MAX), PWM_RANGE_MIN, PWM_RANGE_MAX, 0, OSD_STICK_OVERLAY_HEIGHT) + OSD_STICK_OVERLAY_HEIGHT - 1;

This comment has been minimized.

@ledvinap

ledvinap Dec 6, 2018

Contributor

scaleRange can invert direction, just map to OSD_STICK_OVERLAY_HEIGHT, 0

Also this equation is IMO incorrect, it works only when PWM_RANGE_MAX = 2*PWM_RANGE_MIN

This comment has been minimized.

@pkruger

pkruger Dec 8, 2018

Author Contributor

Scalerange: uint8_t y_pos = (uint8_t)scaleRange(constrain(rcData[vertical_channel], PWM_RANGE_MIN, PWM_RANGE_MAX), PWM_RANGE_MIN, PWM_RANGE_MAX, OSD_STICK_OVERLAY_HEIGHT, 0) + OSD_STICK_OVERLAY_HEIGHT - 1;
Doesn't work, not sure why.

Why do you think it only works with PWM_RANGE_MAX = 2*PWM_RANGE_MIN?

@@ -1236,6 +1313,26 @@ static void osdDrawElements(void)
osdDrawSingleElement(OSD_LOG_STATUS);
}
#endif

#ifdef USE_OSD_STICK_OVERLAY
#ifdef USE_OSD_PROFILES

This comment has been minimized.

@ledvinap

ledvinap Dec 6, 2018

Contributor

This code construction introduces double opening {, confusing some editors (yes, some of us are using emacs ;-)

IMO USE_OSD_PROFILES should override VISIBLE macro(or use some other abstraction), so that OSD profiles are transparent to this type of code

This comment has been minimized.

@pkruger

pkruger Dec 8, 2018

Author Contributor

Oh yes, I like that idea. Done.

@@ -1088,6 +1088,12 @@ const clivalue_t valueTable[] = {
{ "osd_log_status_pos", VAR_UINT16 | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_LOG_STATUS]) },
#endif

#ifdef USE_OSD_STICK_OVERLAY
{ "osd_stick_overlay_left", VAR_UINT16 | MASTER_VALUE, .config.minmax = { 0, OSD_POSCFG_MAX }, PG_OSD_CONFIG, offsetof(osdConfig_t, item_pos[OSD_STICK_OVERLAY_LEFT]) },

This comment has been minimized.

@mikeller

mikeller Dec 8, 2018

Member

These should be following the naming convention for element positions: <element>_pos.

This comment has been minimized.

@pkruger

pkruger Dec 8, 2018

Author Contributor

Done.


## Configuration

Currently (Configurator 10.4.0) OSD Stick Overlays can only be configured via the CLI. Layout your OSD using the configurator and save.

This comment has been minimized.

@mikeller

mikeller Dec 8, 2018

Member

Instead of having this comment here, we should add support for positioning these elements to configurator - the data for it is already sent over MSP, since the entire item_pos array is sent. If we default the TX mode to 2, then this should cover it for most users.

This comment has been minimized.

@pkruger

pkruger Dec 8, 2018

Author Contributor

Ok, I'll remove all references to Configurator 10.4.0 and assume that the Configurator supports it.

for (unsigned x = 0; x < OSD_STICK_OVERLAY_WIDTH; x++) {
for (unsigned y = 0; y < OSD_STICK_OVERLAY_HEIGHT; y++) {
// draw the axes, vertical and horizonal
if ((x == ((OSD_STICK_OVERLAY_WIDTH-1)/2)) && (y == (OSD_STICK_OVERLAY_HEIGHT-1)/2)) {

This comment has been minimized.

@mikeller

mikeller Dec 8, 2018

Member

Spaces around operators are still missing in a lot of places.

This comment has been minimized.

@pkruger

pkruger Dec 8, 2018

Author Contributor

Fixed.

@pkruger pkruger force-pushed the pkruger:3988-feature-add-sticks-verlay-to-in-flight-OSD branch 2 times, most recently from 52451f8 to 541d5d7 Dec 8, 2018

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Dec 8, 2018

Updated with review changes from above.

static statistic_t stats;
static const radioControls_t radioModes[4] = {

This comment has been minimized.

@mikeller

mikeller Dec 8, 2018

Member

This needs to be made conditional, or the build will fail with a warning for targets without USE_OSD_PROFILES. (Protip: Having a look at the result of the CI checks will reveal this type of problem.)

This comment has been minimized.

@pkruger

pkruger Dec 9, 2018

Author Contributor

Thanks, yes I should probably use the CI checks and reap the benefits. Now passing...

@pkruger pkruger force-pushed the pkruger:3988-feature-add-sticks-verlay-to-in-flight-OSD branch from 541d5d7 to ef459cb Dec 9, 2018

@@ -233,3 +243,4 @@ void osdSuppressStats(bool flag);
void setOsdProfile(uint8_t value);
uint8_t getCurrentOsdProfileIndex(void);
void changeOsdProfileIndex(uint8_t profileIndex);
bool osdElementVisible(uint16_t value);

This comment has been minimized.

@mikeller

mikeller Dec 9, 2018

Member

I don't think this is used anywhere externally (or meant to be), so this shouldn't be here.

This comment has been minimized.

@pkruger

pkruger Dec 10, 2018

Author Contributor

It's used in cms.c, i.e. in osd.h VISIBLE(x) is defined as: #define VISIBLE(x) osdElementVisible(x).

@mikeller mikeller merged commit e1e942b into betaflight:master Dec 10, 2018

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Dec 10, 2018

Also added the stick overlays to the OSD Active Elements menu.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 15, 2018

@pkruger: Can I ask you to also open a pull request in configurator to add the stick overlays to the OSD tab?

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 16, 2018

@pkruger: I finally got around to test flying the stick overlays, great work! One thing that I noticed is that with my setup that is cutting off some of the outermost rows of characters on the screen, the stick overlays are covering a large area, in particular they are very high, appearing about twice as high as wide.
I don't know if this is the case on all displays, but I suspect so. How about reducing their height to 5 rows to make them appear more square, at least as an option?

To counteract the resulting loss in vertical resolution, we could change it so that every character (except for the center row) is used for two vertical positions, one with the indicator in the lower half, another one with it in the upper half. This way, we'd practically double the vertical resolution at the expense of 2 extra characters, which sounds like a good deal to me.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.