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

4155 feature for osd elements on a switch #6714

Merged
merged 1 commit into from Dec 3, 2018

Conversation

Projects
None yet
6 participants
@pkruger
Copy link
Contributor

pkruger commented Sep 9, 2018

No description provided.

@pkruger pkruger force-pushed the pkruger:4155-feature-for-osd-elements-on-a-switch branch from 983665c to af216aa Sep 9, 2018

@mikeller
Copy link
Member

mikeller left a comment

I like the approach, and it definitely is something that users are looking for.

There are still a couple of things that will need to be addressed.

But most importantly, this will have to be made conditional to USE_OSD_PROFILES or similar - we are already blowing out the flash size for F3 targets with OSD.

Also, I think @DanNixon was working on OSD profile support already?

Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved src/main/io/osd.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved docs/OSD Profiles.md Outdated
Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/config/feature.h Outdated
@DanNixon

This comment has been minimized.

Copy link
Member

DanNixon commented Sep 9, 2018

I'm not a fan of the custom CLI code (this is also something I dislike about how this feature is implemented in iNav).

IMO my solution of storing OSD config as an array of [flags, x, y], where flags contained three visibility bits for each page was neater.

@ledvinap

This comment has been minimized.

Copy link
Contributor

ledvinap commented Sep 9, 2018

IMO this PR is terrible way to handle this problem. cli code is complicated enough, this will only make things worse.

Simple way to implement this is to change OSD_POSCFG_MAX to 0xffff and let users specify OSD page bits.
Eventually CLI may get better support for composite values (possibly MODE_COMPOSITE and config.composite = osdCompositeInfo)

Show resolved Hide resolved src/main/io/osd.c Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 9, 2018

@DanNixon The custom CLI code is not perfect, I agree. It sounds like your solution also provides 8 possible osd profiles rather than just 4 correct? I would be very interested to see how you implemented yours, I may learn a few new tricks. :o)

In the end I'm all for the cleanest implementation that gives users the most functionality. I just started implementing this because I got impatient and was interested to see how it could be done.

@DanNixon @mikeller Thank you for your comments on this, much appreciated. I have implemented the changes and added replies to your questions. I still have to add USE_OSD_PROFILES - another good point. I will push these changes soon.

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 9, 2018

@ledvinap I like your suggestion of letting the users specify OSD page bits, that may be much simpler. I think there is still a need for CLI support. This implementation solved the problem for me without requiring any Configurator changes - for now. There are other implementations, like one by Dan Nixon which may be better, we may very well end up using that, not sure yet.

@ledvinap

This comment has been minimized.

Copy link
Contributor

ledvinap commented Sep 9, 2018

@pkruger : only CLI code is problematic, the rest is reasonable.

Maybe you can split this PR into OSD handling (just extending CLI range limits) and CLI modification (I'm personally strongly against merging that part)

Also it would be useful to add CLI variable for default OSD profile - that way this feature is useful even without Adjustment range.

@DanNixon

This comment has been minimized.

Copy link
Member

DanNixon commented Sep 9, 2018

@pkruger It could, but shouldn't. I only used three bits for per page visibility (I can't imagine many scenarios where more than 3 pages are required, plus 3 pages fits nicely onto a 3 position switch), leaving 5 bits for future use (configurable precision was another thing I was planning to look at).

#5607 was the start of this, this stores OSD config as an array of [flags, x, y].

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 10, 2018

@DanNixon Good idea. 3 bits make much more sense. I have had mine on a rotary switch which worked, but not great. I think I'd rahter change to 3 bits. Thanks.

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 10, 2018

@ledvinap The CLI variable has been added. The main reason for adding the CLI code is to have some way to configure it without changes to the Configurator. I also don't like the CLI changes btw. Problem is hammering this into an existing framework which doesn't quite fit. I think Dan's idea of separate storage will help clean up the CLI implementation.

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 10, 2018

@mikeller @DanNixon @ledvinap Changes added after code review, thank you for your feedback. This works/does the job, but I must admit the more I look at the CLI changes the more I hate it. It's possible to leave the CLI changes as ledvinap suggested, but it's good to have a backup way to configure it in case the Configurator changes lag a little. I'll have a look at refactoring this taking Dan's suggestion into account and separating the OSD Profile config from OSD Elements.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Sep 10, 2018

@pkruger: Thanks for doing the fixes. Re having the CLI as a fall back, it has to be said that setting up the OSD with CLI only is already tedious, so not having human usable CLI support for OSD profiles won't be making it worse than it is.

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 10, 2018

@mikeller No worries. That's right, I'll have a look at cleaning up the CLI changes, failing that I'll remove them altogether from this PR. (I only did the CLI changes because I didn't want to wait for the Configurator to be updated and needed something to test it with anyway.)

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Sep 10, 2018

@pkruger: Considering that OSD profiles will be hard to set up in CLI only, and the fact that 4.0 is still a relatively long time out, I would say if we get this into the firmware soon, and then get configurator support for it going soon after, there will be enough time to get this tested and stabilised sufficiently so that it can be released in 4.0 with configurator support.

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 11, 2018

@mikeller Thanks, that's great news. I have since reworked the CLI - it is much better now, or at least I think so. No more special handling etc. Although this now requires dedicated storage for each OSD element profile configuration, it has the added benefit of standard CLI as well as not losing the profile configuration if anything is changed on the OSD tab in the configurator. I think this will make the configurator changes simpler as well. I hope this solution is better for everyone. (@DanNixon @ledvinap)

@DanNixon
Copy link
Member

DanNixon left a comment

This is not quite what I described. See my other PR that I linked for how I would lay out OSD storage. Currently you are using a lot of space juts for the additional CLI strings you introduced and there is still custom CLI code that shouldn't need to exist.

Show resolved Hide resolved src/main/fc/config.h Outdated
@ledvinap
Copy link
Contributor

ledvinap left a comment

Please see @DanNixon PR ...
osdprofile cli is still overkill ...

Show resolved Hide resolved src/main/fc/config.c Outdated
Show resolved Hide resolved src/main/fc/config.h Outdated
Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/fc/rc_adjustments.c
Show resolved Hide resolved src/main/io/osd.c Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated
Show resolved Hide resolved src/main/interface/cli.c Outdated

@pkruger pkruger force-pushed the pkruger:4155-feature-for-osd-elements-on-a-switch branch from 04b892a to 6ef90a4 Sep 11, 2018

@DanNixon
Copy link
Member

DanNixon left a comment

I'd vote for renaming this "pages" rather than "profile". Profile implies more configuration than what you can actually do, all this can achieve is three combinations of elements that can be visible at once.

Show resolved Hide resolved src/main/io/osd.h Outdated
@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Sep 11, 2018

@DanNixon I disagree on naming it osd pages. Users are familiar with pid & rate profiles and have already called it this in the initial issue. Also it aligns better with existing profile commands in the CLI.
You'll notice I have carried over your osd element storage changes and removed the custom CLI changes. I really like the X,Y coordinates for the osd, makes a lot more sense that the previous method.
I have temporarily added an encoding for the MSP setting of the X,Y and flag values, maybe something like this?

                // Below encoding is still to be decided
                // 15   14  13  12  11  10  9   8   7   6   5   4   3   2   1   0
                // UU   UU  UU   X   X   X  X   X   Y   Y   Y   Y   P   P   P   V
                // where:
                // UU = Unused
                //  X = bits 12-8 making up bits 5-0 of the osd element x position
                //  Y = bits 7-4 making up bits 4-0 of the osd element y position
                //  P = bits 3-1 making up bits 0-2 of the osd profile
                //  V = bit 0, visibility bit, 0=off, 1=on

Also included the changes outlined by @ledvinap above.

@DanNixon

This comment has been minimized.

Copy link
Member

DanNixon commented Sep 11, 2018

We can leave the name to a vote, but this is not a profile in the same way PID and rate are and naming it as such may lead to misconceptions about how it works.

If the storage is changing then the MSP API should too IMO, then if the additional bits are used or a new device needs more coordinate space then less changes are required. You also need to remove the visibility bit.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Sep 13, 2018

@DanNixon: The MSP API version is managed centrally and in synch with releases, i.e. 4.0 where this is scheduled to be in will have its own MSP API version (currently planned to be 1.41).

Show resolved Hide resolved src/main/cms/cms.c Outdated
Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/interface/settings.c Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
@@ -87,6 +89,18 @@ PG_RESET_TEMPLATE(systemConfig_t, systemConfig,
.powerOnArmingGraceTime = 5,
.boardIdentifier = TARGET_BOARD_IDENTIFIER
);
#else

This comment has been minimized.

@ledvinap

ledvinap Sep 14, 2018

Contributor

This code is no longer necessary

This comment has been minimized.

@pkruger

pkruger Sep 18, 2018

Author Contributor

Fixed.

Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/interface/msp.c Outdated
Show resolved Hide resolved src/main/interface/msp.c Outdated
Show resolved Hide resolved src/main/interface/msp.c Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated
Show resolved Hide resolved src/main/io/osd.h
Show resolved Hide resolved src/main/io/osd.h Outdated
@ledvinap

This comment has been minimized.

Copy link
Contributor

ledvinap commented Sep 18, 2018

@pkruger : Using accessor macros for OSD element will make it trivial to change internal representation in future (pack elements in 16 bits).
Another possibility is to use struct bitfields, but it isn't common coding style in BF

@mikeller
Copy link
Member

mikeller left a comment

Good work on the documentation! Only a few minor changes left.

Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/io/osd.h Outdated

@pkruger pkruger force-pushed the pkruger:4155-feature-for-osd-elements-on-a-switch branch 2 times, most recently from 1fd31d5 to ac0afab Nov 10, 2018

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Nov 10, 2018

All changes mentioned above done - getting close now...

Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated

@pkruger pkruger force-pushed the pkruger:4155-feature-for-osd-elements-on-a-switch branch 3 times, most recently from 08f8905 to ce86780 Nov 12, 2018

Show resolved Hide resolved src/main/fc/rc_adjustments.c Outdated
Show resolved Hide resolved src/main/fc/rc_adjustments.h Outdated

@pkruger pkruger force-pushed the pkruger:4155-feature-for-osd-elements-on-a-switch branch from ce86780 to dde32d1 Nov 29, 2018

@mikeller
Copy link
Member

mikeller left a comment

Nice!

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Nov 29, 2018

I have applied the OSD Profile feature on top of the current master and retested - it works! (No dummy/filler entries in adjustmentFunction_e required and also the position of the OSD Profiles entry in defaultAdjustmentConfigs does not need to correspond to its number in adjustmentFunction_e .) I must have made a mistake somewhere when testing it before. It also includes the above changes required by ledvinap and etracer65.

Close now - I think. :o) Thanks for all your input along the way, this feature is in a much better state now than the initial PR.

@mikeller
Copy link
Member

mikeller left a comment

There are also two targets that define VISIBLE_FLAG in config.c, which needs to be changed.

Show resolved Hide resolved src/main/fc/rc_adjustments.c

@pkruger pkruger force-pushed the pkruger:4155-feature-for-osd-elements-on-a-switch branch from dde32d1 to 8d981df Dec 1, 2018

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 1, 2018

@DanNixon, @ledvinap: Have your concerns with this pull request been addressed?

@ledvinap
Copy link
Contributor

ledvinap left a comment

Thanks for patience

Outdated.

@mikeller mikeller merged commit b6408b3 into betaflight:master Dec 3, 2018

@pkruger

This comment has been minimized.

Copy link
Contributor Author

pkruger commented Dec 4, 2018

Awesome...

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 5, 2018

Fixes #4155.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 5, 2018

@pkruger: Now that this is done, can I get you interested in adding support for this to the configurator? I think for users to be able to set up this feature it will be important to have a GUI for it.
This could be done by adding a matrix of checkboxes for each (vertical) profile and (horizontal) OSD element to the left side of the OSD tab, plus a slider up top that the user can use to select which profile is shown in the preview.

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.