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

Modified OSD stats functions to accomodate single page stats for HD #8649

Closed

Conversation

M0j0Risin
Copy link
Contributor

SnapShot

Implements a single function "osdShowStats()" for showing stats with parameters for whether the display is HD and which page to show if it is SD. So, existing paging functionality will be maintained for SD displays and a single stats page will be shown on HD displays.

@Jetrell
Copy link

Jetrell commented Dec 27, 2022

@M0j0Risin I assume this was written with DJI? You may want to take into account the Video_lines height for other HD systems as well.

 VIDEO_LINES: 
        HDZERO: 18
        DJIWTF: 22
        AVATAR: 20
        BF43COMPAT: 16

It will be a squeeze for HDzero, And as seen in the image below, using Avatar. The Stats cover the factory OSD data.

20221227_110050

@M0j0Risin
Copy link
Contributor Author

@Jetrell Ok, good to know. Thanks for the heads up. I made an assumption that if osdDisplayIsHD() returned true that they'd all have at least the number of lines as HDZero. But I see what you mean about it being a tight squeeze. I think it could be shifted up one line to solve that for HDZero, but that's not going to solve it for BF43COMPAT since it currently takes 18 lines to display.

Since my main goal was to consolidate them on to one screen for DJIWTF to eliminate the problem of the second page of stats being trimmed off in post process OSD overlay, I suppose one solution would be simply to modify the function param as "isSinglePage" instead of "isHD" and simply keep the multiple pages for anything smaller than VIDEO_BUFFER_CHARS_DJIWTF.

…" and added a function osdDisplayIsSinglePageCompatible() to check if the screen size is at least the size for DJIWTF.
@MrD-RC
Copy link
Collaborator

MrD-RC commented Dec 27, 2022

They do. HDZero has 18 lines, so that should be the limit. I think if the DJI hack for O3 is used, it should be treated as SD. So maybe just add that to the check.

@M0j0Risin
Copy link
Contributor Author

M0j0Risin commented Dec 27, 2022

I guess have a look at that last commit. That should make it so it does single page for DJIWTF only. There isn't a "VIDEO_BUFFER_CHARS" definition for Avatar, so I'm not really sure where that falls in there. For HDZERO, it could be added back to the osdIsSinglePageCompatible() function, but the stats may need to be shifted up one row.

I haven't tried this yet, but I assume this would work...

static void osdShowStats(bool isSinglePageCompatible, int page) { ... uint8_t top = 1; /* first fully visible line */ if (isSinglePageCompatible) top = 0; ...

@MrD-RC
Copy link
Collaborator

MrD-RC commented Dec 27, 2022

I don't think doing this just for WTF is right. Perhaps the page can be reconfigured so that it fits on 18 rows, as there is also more width.

@M0j0Risin
Copy link
Contributor Author

M0j0Risin commented Dec 27, 2022

I don't think doing this just for WTF is right. Perhaps the page can be reconfigured so that it fits on 18 rows, as there is also more width.

I can try to do that. Move it into multiple columns. Shouldn't be too hard.

Of course the trick is going to be keeping it centered enough so it doesn't overlap other hard-wired OSD elements in the goggles. At least looking at Jarrel's picture above and what's happening with DJIWTF it's all on the bottom or justified to the right. So I guess having it biased to the left would be the best.

Above is the number of horizontal lines for each. Do we have those numbers for vertical lines as well?

@M0j0Risin
Copy link
Contributor Author

Or I suppose another option could be to do like the "Min/Max Z G-Force" line and combine a few into one. Max/Avg speed and Min RSSI %/DBM for example.

@MrD-RC
Copy link
Collaborator

MrD-RC commented Dec 27, 2022

That's what I wad thinking. All the GForce info on one line. There was something else that I thought could go on a single line. Perhaps RSSI % and dBm.

Updated the function which decides whether to support single page stats, osdVideoSystemIsSinglePageStatsCompatible(), to be based on the enumerated value for the selected video system.

Combined MAX SPEED and AVG SPEED on to one line.

Combined MIN RSSI % and MIN RSSI DBM on to one line.

So we're down to 16 lines, 17 including the blank line at the top.
@M0j0Risin
Copy link
Contributor Author

stats_snapshot

Ok, here's another go at it.

Updated the function which decides whether to support single page stats, osdVideoSystemIsSinglePageStatsCompatible(), to be based on the enumerated value for the selected video system.

Combined MAX SPEED and AVG SPEED on to one line.

Combined MIN RSSI % and MIN RSSI DBM on to one line.

So we're down to 16 lines, 17 including the blank line at the top.

I'm still a little unclear where Walksnail/Avatar falls into this as there is no enumerated video system type for that. I'm assuming they're choosing HDZERO on the OSD tab, so this should cover them for now.

@M0j0Risin
Copy link
Contributor Author

Now that I look at it again, I think that first line should probably read "SPEED MAX/AVG" to be more consistent with the other multi-valued lines.

@MrD-RC
Copy link
Collaborator

MrD-RC commented Dec 28, 2022

Avatar is 20 I think. It's certainly no less that HDZero.

I'm still working on that PR currently. To be honest, on the bench everything is fine. I just want to test in flight too. Mostly for what happens after loss of data after obstacles etc. I could publish it with a requires testing label to be honest.

@M0j0Risin
Copy link
Contributor Author

lol. I shouldn't be coding while I'm tired. I changed it to SPEED MAX/AVG, but that makes no sense. I don't know what I was thinking looking at it before.

@M0j0Risin
Copy link
Contributor Author

Avatar is 20 I think. It's certainly no less that HDZero.

I'm still working on that PR currently. To be honest, on the bench everything is fine. I just want to test in flight too. Mostly for what happens after loss of data after obstacles etc. I could publish it with a requires testing label to be honest.

It's more a question of which video system do they select on the OSD tab. The function I wrote uses a value defined in this enumeration. So only VIDEO_SYSTEM_HDZERO and VIDEO_SYSTEM_DJIWTF allow single page stats. Will there be another entry here for Avatar?

typedef enum { VIDEO_SYSTEM_AUTO = 0, VIDEO_SYSTEM_PAL, VIDEO_SYSTEM_NTSC, VIDEO_SYSTEM_HDZERO, VIDEO_SYSTEM_DJIWTF, VIDEO_SYSTEM_BFCOMPAT } videoSystem_e;

@MrD-RC
Copy link
Collaborator

MrD-RC commented Dec 28, 2022

Currently HDZero. But in 6.0 RC 1 it will have its own option.

There will be a VIDEO_SYSTEM_AVATAR.

The width is different too, at 53 columns. So nice, centrally aligned crosshairs.

…rt Avatar once it is added to the videoSystem_e enumeration.
@M0j0Risin
Copy link
Contributor Author

Currently HDZero. But in 6.0 RC 1 it will have its own option.

There will be a VIDEO_SYSTEM_AVATAR.

The width is different too, at 53 columns. So nice, centrally aligned crosshairs.

I see that you did add that in your latest pull request. Once that is merged, I'll sync that and make an additional change here to add VIDEO_SYSTEM_AVATAR as a supported video system for single-page stats. I know you mentioned that there may be an addition of a line for vibration stats, so we can do that too. If it fits, great! But if we need to condense another line, I think MAX CURRENT and MAX POWER might make sense to squeeze into one line as well. And of course, the whole shebang could be moved up one line to the top of the screen if absolutely necessary.

I also corrected some buggy behavior that was happening surrounding the stick commands to change pages for multi-page stats.
…events so that the "Saving Settings" and "Settings Saved" messages have a chance to be displayed since it wouldn't redraw them normally.
@M0j0Risin
Copy link
Contributor Author

SnapShot
I fixed the merge conflicts with PR #8439.

I also corrected some buggy behavior that was happening surrounding the stick commands to change pages for multi-page stats. It could be related to timing with MSP DisplayPort and DJIWTF, so I'm not certain whether that impacts analog as well, but I figured it's probably worth fixing regardless. What would basically happen was that holding the roll stick for any length of time would cause a kind of race condition where the method to show the stats would be called multiple times on top of itself resulting in some glitchy behavior where some of the text wouldn't get displayed.

I kind of mimicked some other code I saw in 'cms.c' to have it require the sticks be held to the ends for a few milliseconds and that seems to have solved that.

Copy link
Collaborator

@MrD-RC MrD-RC left a comment

Choose a reason for hiding this comment

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

I have a couple of suggestions. See what you think?

src/main/io/osd.c Outdated Show resolved Hide resolved
src/main/io/osd.c Outdated Show resolved Hide resolved
…ly on the number of rows currently displayed. OSD_STATS_SINGLE_PAGE_MIN_ROWS stores the minimum number of rows for single-page stats.

I also made some additional improvements surrounding the logic for manually paging using the stick commands for multi-page stats. I think this makes the code a lot easier to follow.
@Jetrell
Copy link

Jetrell commented Jan 27, 2023

@M0j0Risin I tested your last build with analogue (PAL) and with Avatar. Both seem to work fine.

I tried replicating the issues you spoke of. in your earlier commits. By holding the aileron stick, or selecting various different multi-page timers.. Analogue worked as it should.

Avatar.
Avatar test

@M0j0Risin
Copy link
Contributor Author

@Jetrell Thanks for the update. I was just looking back at this today to see whatever came of it. I did all of this while I was on vacation over Christmas and haven't really checked on it since. With the release of RC1 today I was curious if it actually made it in there. Doesn't look like it has. I'm assuming because there are some conflicts that arose since then. I'm not really even sure if there's any interest in trying to resolve it.

@M0j0Risin I tested your last build with analogue (PAL) and with Avatar. Both seem to work fine.

I tried replicating the issues you spoke of. in your earlier commits. By holding the aileron stick, or selecting various different multi-page timers.. Analogue worked as it should.

Avatar. Avatar test

@Jetrell
Copy link

Jetrell commented Jan 30, 2023

I'm assuming because there are some conflicts that arose since then. I'm not really even sure if there's any interest in trying to resolve it.

I'm not sure if there are any conflicts. Maybe only with HDzero.. It only has factory OSD characters on the top line. So it should work fine.
I like the idea of having all the stats on a single page.. I'm not a big fan of the two page stats.. It only came about in analogue because of NTSC.
It may have just been overlooked, because of the business of preparing the 6.0 release.

@MrD-RC
Copy link
Collaborator

MrD-RC commented Jan 30, 2023

There are some conficts in the code. This will be because of other code that has been merged. They shouldn't take much to resolve.

I think this would be a great addition too.

@M0j0Risin
Copy link
Contributor Author

There are some conficts in the code. This will be because of other code that has been merged. They shouldn't take much to resolve.

I think this would be a great addition too.

Ok then. I'll try to get to resolving it this week. I'm kind of in a time crunch with other work stuff, but I think this should be pretty simple so I should be able to sort it out.

@M0j0Risin
Copy link
Contributor Author

In resolving the conflicts with RC1 I somehow got my repo out of sync with this pull request. So I'll close it and open another.

@M0j0Risin M0j0Risin closed this Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants