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

Compatiable for DJI O3 #9384

Closed
wants to merge 1 commit into from
Closed

Conversation

xuhao1
Copy link

@xuhao1 xuhao1 commented Oct 21, 2023

Better compatibility for DJI O3, including:

  1. G force: for 3d g forces, use single 'G'
  2. Transmitter power, use 'MW'
  3. Roll and pitch, use PIT or ROL
  4. Fly distance
  5. Heading

Some is not perfect, but I believe they are better than '?'

Before
image

After
image

Performed fly test on DJI-O3 system with SpeedyBee F405Wing on BF 6.1.1, cherry pick to 7.0 and verified on ground.

case SYM_PITCH_DOWN:
return BF_SYM_PITCH_DOWN;
return BF_SYM_PITCH;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is ambiguous. DJI should fix the font support.

Copy link
Author

@xuhao1 xuhao1 Oct 21, 2023

Choose a reason for hiding this comment

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

@mmosca Yes, but this patch should better than '?' since we can't push DJI to do this. May be I should add a minus here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really. Ambiguity can be worse than a question mark.
You will never get full coverage of the OSD with O3, unless DJI implements support for INAV's font.

case SYM_HEADING:
return BF_SYM_HEADING;
return BF_SYM_ARROW_NORTH;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is trying a bit too hard. The inav icon represents a compass, this is picking an arrow pointing up.

Copy link
Author

Choose a reason for hiding this comment

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

@mmosca But BF do not have something looks like "Compass", so we may choose something like char 'C' or just arrow to represent following number is for heading.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mmosca But BF do not have something looks like "Compass", so we may choose something like char 'C' or just arrow to represent following number is for heading.

There are a lot of symbols not included in BF. The goal for the BFCOMPAT modes is not to have 0 question marks, but to map the symbols that betaflight actually supports and make it clear when a symbol is not supported, without adding too much complexity to the code, or ambiguity to the OSD.

Unfortunatelly, only DJI can actually fix the overall situation. :(

@@ -2385,7 +2385,12 @@ static bool osdDrawSingleElement(uint8_t item)
if (!failsafeIsReceivingRxData())
tfp_sprintf(buff, "%s%c", " ", SYM_BLANK);
else
tfp_sprintf(buff, "%4d%c", rxLinkStatistics.uplinkTXPower, SYM_MW);
if (isBfCompatibleVideoSystem(osdConfig())) {
tfp_sprintf(buff, "%4dMW", rxLinkStatistics.uplinkTXPower);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have not added units like this to avoid overcomplicating the osd code even more.

@@ -186,17 +186,17 @@ uint8_t getBfCharacter(uint8_t ch, uint8_t page)

case SYM_ALT_M:
return BF_SYM_M;

case SYM_TOTAL:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably a valid suggestion.

@mmosca
Copy link
Collaborator

mmosca commented Oct 21, 2023

I think overall, SYM_TOTAL sugestion is valid and can be merged, but I am not sold on the other items.
As a general rule of thumb, the idea of the beta flight compatibility is to keep it simple and limited to direct 1:1 matches.
The ideal fix would be for DJI to implement support for INAV's font, so we can drop the compatibility option.

We certainly don't want to change units to be fully spelled out, as that opens a can of worms and will add even more complexity to the main osd code, and not only on the bf_compat file.

@mmosca
Copy link
Collaborator

mmosca commented Oct 21, 2023

Also, some of the proposed introduce ambiguity(pitch up/down sharing the same symbol, for example), which is not good.
The '?' makes it clear that the osd element is not fully supported with O3/DJI.


case SYM_GFORCE:
return BF_SYM_GFORCE;
return 'G';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am on the fence on this one. @MrD-RC, @DzikuVx ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’ve left my thoughts below. I don’t think it will surprise you much 😉

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am obviously a bit more accommodating there. Using G for the icon is a simple enough change, and since it did not change the symbol for the other axii, I am leaving towards it being ok to change.

@MrD-RC
Copy link
Collaborator

MrD-RC commented Oct 21, 2023

Honestly, I think there have been enough changes for DJI. It’s about time they supported their customers. Instead of spending time and money trying to push competitors out of the market.

@mmosca
Copy link
Collaborator

mmosca commented Oct 21, 2023

@xuhao1 if you update the pull request to have only the SYM_TOTAL change and perhaps the G force one it will be more likely to be merged.

Otherwise, if not, I can probably submit a pull request for the SYM_TOTAL on your behalf.

@xuhao1
Copy link
Author

xuhao1 commented Oct 21, 2023

@xuhao1 if you update the pull request to have only the SYM_TOTAL change and perhaps the G force one it will be more likely to be merged.

Otherwise, if not, I can probably submit a pull request for the SYM_TOTAL on your behalf.

OK, I will do so. And I think this one is also OK.

 case SYM_ALT_KM:
        return BF_SYM_KM;

In
https://github.com/iNavFlight/inav/pull/9384/files#diff-6ea472db54a67d4190a0add45a955755c7968aa1816046b3699d1bf490d9e564R198

@mmosca
Copy link
Collaborator

mmosca commented Oct 21, 2023

@xuhao1 if you update the pull request to have only the SYM_TOTAL change and perhaps the G force one it will be more likely to be merged.
Otherwise, if not, I can probably submit a pull request for the SYM_TOTAL on your behalf.

OK, I will do so. And I think this one is also OK.

 case SYM_ALT_KM:
        return BF_SYM_KM;

In https://github.com/iNavFlight/inav/pull/9384/files#diff-6ea472db54a67d4190a0add45a955755c7968aa1816046b3699d1bf490d9e564R198

Please don't. This is likely to fall into the ambiguity criteria and even if accepted, would be an incomplete fix, as we have other units as well.

@mmosca
Copy link
Collaborator

mmosca commented Oct 22, 2023

I merged the changes to SYM_TOTAL and GFORCE in a different pull request.

Thanks for the pull request. :)

@mmosca mmosca closed this Oct 22, 2023
@DzikuVx DzikuVx added this to the 7.0 milestone Oct 23, 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.

4 participants