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

Revert "Improvements to BFCOMPAT character and icon mappings" #8928

Merged

mmosca merged 1 commit into release_6.1.0 from revert-8926-bfcompat_mods2

Mar 30, 2023

Collaborator

mmosca commented Mar 30, 2023

Temporary Reverts #8926 so we can address some issues.


          Revert "Improvements to BFCOMPAT character and icon mappings"

c87d641

Collaborator Author

mmosca commented Mar 30, 2023

The original pull request was based on the broken DJI font, not on BF specs.

We risk getting things broken in the future if we include this and it will be an INAV bug in the future.

If we follow BF's specs, it is DJI's bug which I hope they will fix, since they seem to be in closer colaboration with Betaflight than us.

mmosca commented

View reviewed changes

src/main/io/bf_osd_symbols.h

               // GPS and navigation
               #define BF_SYM_LAT                     0x89
               #define BF_SYM_LON                     0x98
               #define BF_SYM_ALTITUDE                0x7F
-              #define BF_SYM_TOTAL_DISTANCE          0x2A
+              #define BF_SYM_TOTAL_DISTANCE          0x71

Collaborator Author

mmosca Mar 30, 2023

0x71 is the correct value for SYM_TOTALDISTANCE in betaflight. This is an issue with the DJI font.

Contributor

rmaia3d Mar 30, 2023

I think we should follow what DJI implemented, not what BF proposed. So, unless DJI changes it, this is the "correct" value IMO.

Collaborator

MrD-RC Mar 30, 2023

No, we should follow BetaFlight. DJI are using the old BF font. However, they have got parts of it wrong. I 100% agree with @mmosca. This is a BetaFlight compatibility mode. Therefore is should be to the BF spec. DJI should fix their errors.

Well, lets face it. DJI should really do all this properly, and BF compatibility modes shouldn’t be required.

mmosca commented

View reviewed changes

src/main/io/bf_osd_symbols.h

@@ @@ -32,14 +32,12 @@ @@
               #define BF_SYM_ROLL                    0x14
               #define BF_SYM_PITCH                   0x15
               #define BF_SYM_TEMPERATURE             0x7A
-              #define BF_SYM_MAX                     0x24

Collaborator Author

mmosca Mar 30, 2023

0x24 and 0x7C are not used by betaflight and only exist in DJI's font.

This will likely break in the future.

Contributor

rmaia3d Mar 30, 2023

Again, DJI's actual implementation should be the "north", not the original BF proposal. If it breaks in the future, no big deal, we change the mapping value to whatever DJI implements/changes.

mmosca commented

View reviewed changes

src/main/io/bf_osd_symbols.h

@@ @@ -140,8 +138,6 @@ @@
               // Time
               #define BF_SYM_ON_M                    0x9B
               #define BF_SYM_FLY_M                   0x9C
-              #define BF_SYM_ON_H                    0x70

Collaborator Author

mmosca Mar 30, 2023

These constants are not in betaflight's font. 0x70 is BF_SYM_SPEED and 0x71 is BF_SYM_TOTAL_DISTANCE

Contributor

rmaia3d Mar 30, 2023

One more instance of not seeing the point of being "chained" to BF's proposal. The fonts are there on DJI's table for us to use it. Let's use it!

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

@@ @@ -38,7 +38,7 @@ uint8_t getBfCharacter(uint8_t ch, uint8_t page) @@
                       return BF_SYM_RSSI;
                   case SYM_LQ:
-                      return BF_SYM_AH_LEFT;

Collaborator Author

mmosca Mar 30, 2023

BF_SYM_QUALITY is the correct value here. It is a bug in DJI's font.

Contributor

rmaia3d Mar 30, 2023

Yeah, this one I chose a symbolic replacement from what was available. I do agree the correct symbol would be much better, but let's work with what we have right now.

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

		case SYM_HEADING:
		return BF_SYM_OVER_HOME;

Collaborator Author

mmosca Mar 30, 2023

This display's a house icon, Inav's icon is a compass.

I am not sure the semantics here match.

Contributor

rmaia3d Mar 30, 2023

The original intention might have been a house symbol, but visually it's actually a very nice looking upwards pointing arrowhead! Since there is no compass icon amongst DJI's table, this was the closest replacement available IMO.

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

		case SYM_HDP_L:
		return 'H';

Collaborator Author

mmosca Mar 30, 2023

This one may be ok. Open for discussions.

HP has a meaning though, horse power, so does HD.

Contributor

rmaia3d Mar 30, 2023

Yeah, I agree that HP (or HD) aren't really the best abbreviations. But in the spirit of not changing the field's formatting to fit extra letters, IMO this is a suitable approximation. I know we won't get to the ideal case (which would be all the proper, customizable, icons), but the intention is to get as close as possible to that. Some compromises will be needed, though.

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

		case SYM_DB:
		return 'D'; // Just D to resemble "dB"

Collaborator Author

mmosca Mar 30, 2023

Not sold on replacing SYM_DB with 'D' and SYM_DBM with blank.

The idea of displaying '?' is to highlight where the problems are. We don't want to hide them.

Contributor

rmaia3d Mar 30, 2023

Yeah, replacing dB with just "D" isn't the ideal solution, I agree. But I think it's definitely better than "?". Also, the previous symbol being used for dBm already displayed as blank, I just made it clear. No suitable replacement or letter abbreviation for dBm I could find.

I don't agree at all the "?" should be kept. Highlighting the problems to the developers is one thing, I totally agree with that. Highlighting it for the end user, is pointless. It gives the impression of a buggy and unfinished product. In the initial rush to get the OSD working for the O3 users, it was an acceptable compromise. But I believe enough time has passed for the implementation to become more mature and "polished". Specially when it's just a simple matter of character remapping to greatly improve the user experience.

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

                   case SYM_AH_DECORATION_DOWN:
-                      return BF_SYM_AH_DECORATION;
-              */
-                  case SYM_DIRECTION:

Collaborator Author

mmosca Mar 30, 2023

See my comment on the header file.
Arrow up vs house.

Contributor

rmaia3d Mar 30, 2023

Same as my previous comment. It might be intended as a house, but visually, it's a perfect arrowhead. And since the objective here is visual representation, I believe this fits perfectly the usecase.

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

		case SYM_MAH_KM_0:
		return BF_SYM_MAH;

Collaborator Author

mmosca Mar 30, 2023

Adds too much room for confusion.

We already use BF_SYM_MAH

Contributor

rmaia3d Mar 30, 2023

This is to be used together with the letter that comes right beside it. Indicating "mAh per kilometer". Yeah, it's a bit of a strech, but the best that I could come up with given the limitations of field formatting and the implemented character table.

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c


		case SYM_MAH_KM_1:
		return 'K'; // K indicating Km

Collaborator Author

mmosca Mar 30, 2023

Too vague. Not sold on replacing symbols with single letters.

Contributor

rmaia3d Mar 30, 2023

Only way I could think of to indicate Km. Agree it's not ideal, but much better than "??", wouldn't you agree?

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

		case SYM_MW:
		return '^'; // Power symbol for math

Collaborator Author

mmosca Mar 30, 2023

Long shot. I think we are trying too hard.

Contributor

rmaia3d Mar 30, 2023

Agree it's a long shot. But do you have a better suggestion other than "?" ?

mmosca commented

View reviewed changes

src/main/io/displayport_msp_bf_compat.c

		case SYM_HOME_DIST:
		return BF_SYM_HOMEFLAG;

Collaborator Author

mmosca Mar 30, 2023

This one is probably ok. Pin represeting home.

Contributor

rmaia3d Mar 30, 2023

Yep! It's the only available "home" icon in the font table too.

mmosca merged commit c48a884 into release_6.1.0

Contributor

rmaia3d commented Mar 30, 2023

The original pull request was based on the broken DJI font, not on BF specs.

We risk getting things broken in the future if we include this and it will be an INAV bug in the future.

If we follow BF's specs, it is DJI's bug which I hope they will fix, since they seem to be in closer colaboration with Betaflight than us.

In my opinion, I don't see a point in following the BF proposal exactly. I believe we should follow what DJI actually implemented, not what was proposed to them.

Yes, they may "fix" it according to what BF asks of them. But how long could that take? Or they may not even fix it and we will be waiting forever with an OSD full of "??" which looks totally buggy...

If they do fix, how long would they take to do it? Why can't we use what's already implemented by them, and, if they change, we change ourselves too? It's only a visual remapping, nothing critical if it gets "broken". And also a very easy fix, IMO.

So I really don't see this as an issue. At all.

Contributor

rmaia3d commented Mar 30, 2023

For illustration, here's how two OSD layouts look like with the remapping improvements. For the fields being used, no "??" at all, and a much more intuitive and pleasing visual experience.

rmaia3d added a commit to rmaia3d/inav that referenced this pull request


          Revert "Merge pull request iNavFlight#8928 from iNavFlight/revert-892…

…6-bfcompat_mods2"

This reverts commit c48a884, reversing
changes made to 0e4eead.

mmosca deleted the revert-8926-bfcompat_mods2 branch

June 13, 2023 08:12

rmaia3d mentioned this pull request

BFCOMPAT - fix power field scalings and improve auto throttle labels #9389

Closed

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

Labels

None yet