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

Mixer refactoring - make FC unaware of anything besides mixer rules #2978

Merged
merged 19 commits into from
Apr 21, 2018

Conversation

DzikuVx
Copy link
Member

@DzikuVx DzikuVx commented Mar 27, 2018

Mixer rules will be applied by Configurator via MSP instead of having predefined presets

So that we know where those changes are going, very "work in progress" will require also Configurator support

@DzikuVx DzikuVx changed the title Platform type and hasFlaps flag moved to user settings Mixer refactoring - make FC unaware of anything besides mixer rules Mar 27, 2018
@DzikuVx DzikuVx requested review from martinbudden and removed request for martinbudden March 27, 2018 19:29
@DzikuVx
Copy link
Member Author

DzikuVx commented Mar 28, 2018

This is getting bigger than I expected... so far -620 lines of code

@stronnag
Copy link
Collaborator

stronnag commented Mar 28, 2018

so in the CLI, mixer will disappear, but mmixand smix will remain (I hope)?

@DzikuVx
Copy link
Member Author

DzikuVx commented Mar 28, 2018

Yes, mmix and smix will remain unaffected @stronnag
I wonder how much flash and memory will be saved thanks to that in the end

@DzikuVx
Copy link
Member Author

DzikuVx commented Mar 28, 2018

New MSP2 frames specification in https://github.com/iNavFlight/inav/wiki/INAV-MSP-frames-changelog

@DzikuVx
Copy link
Member Author

DzikuVx commented Apr 5, 2018

Requires iNavFlight/inav-configurator#332

@stronnag
Copy link
Collaborator

The js preset looks correct, so it's probably an incorrect assumption in the MSP2 constraints?
As it sets correctly from the CLI, I will fly it later, weather permitting.

@DzikuVx
Copy link
Member Author

DzikuVx commented Apr 11, 2018

@stronnag I've increased allowed range in MSP and editing to -2/2 (-1/1) previously. Both FC and configurator. You might give it a try. I did not tested tho, not FC at work :(

@stronnag
Copy link
Collaborator

Working consistently on the bench between CLI and configurator setups. Flight test in a couple of hours time.

@stronnag
Copy link
Collaborator

flight tested OK (tricopter).

@stronnag
Copy link
Collaborator

stronnag commented Apr 11, 2018

Can we have a MSP API version-up please (and maybe a configuration new version too)?
Got it ...

Copy link
Member

@digitalentity digitalentity left a comment

Choose a reason for hiding this comment

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

Looks ok to me

@@ -1016,23 +1002,6 @@ static void cliMotorMix(char *cmdline)
for (uint32_t i = 0; i < MAX_SUPPORTED_MOTORS; i++) {
customMotorMixerMutable(i)->throttle = 0.0f;
}
} else if (sl_strncasecmp(cmdline, "load", 4) == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd still keep some common presets inside the firmware. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just checked statistics. circa 60% of cases are QuadX. Maybe we shouyld set default mmix to QuadX. But I'd avoid keeping presets in firmware at all. Then half would be here, half there, IDs would be kept in sync... I really doubt anyone uses INAV without Configurator and CLI backup/restore is working just fine

Copy link
Collaborator

Choose a reason for hiding this comment

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

While it is certainly the case that one can configure the new mixer stuff in the CLI, do diff backup, reflash, restore and have a working machine, one thing you can't do (anymore) at least for non-QuadX, is:

  • Set up the platform / mixer in the configurator with nice 3D model etc.
  • Save a CLI 'diff'
  • Restore the diff including defaults noreboot
  • Go back to the configurator and have the 3D model displayed.

This is because while the platform type is persisted in the CLI, the preset id is not.

I appreciate this is just pretty 'UX' and of no technical consequence (and possibly of no concern to > 60% of the user base).

I'm also mildly (very mildly) concerned that the new mixer aware configurator does not maintain backwards compatibility with earlier firmware; on the other hand, this could be an opportunity to rip out a pile of legacy compatibility code in the configurator.

Notwithstanding the technical merit of this change, I can see a PR (public relations) / UX hiatus in the project's future, in particular with regard to 3rd party configuration tools and perhaps external OSDs. "omg you're killed another cute kitten android developer".

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are right about restore @stronnag . I will address this issue in a next few commits.
@stronnag modified configurator fully supports previous firmware versions. But problem of backward compatibility is not that big as you might imagine.

On the other hand, I'm not affarid about 3rd party tools. New firmware still reports mixer (always QuadX) and other things. What you can not do is to setup platform. I doubt users really use android tool to do full setup.

But preset setting will have to be present in CLI after all. And Configurator will also display more messages / aids.

@@ -90,6 +90,9 @@ tables:
- name: smartport_fuel_unit
values: ["PERCENT", "MAH", "MWH"]
enum: smartportFuelUnit_e
- name: platform_type
values: ["MULTIROTOR", "AIRPLANE", "HELICOPTER", "TRICOPTER", "ROVER", "BOAT"]
Copy link
Member

Choose a reason for hiding this comment

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

Yay! I like that! Maybe VTOL's should have separate platform types?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would make sense, right?

@stronnag
Copy link
Collaborator

stronnag commented Apr 13, 2018

Thanks for that, cli around trip working well with 'minority mixer models' shown in the configurator.
I will fly it again today, with the vtail.

@stronnag
Copy link
Collaborator

stronnag commented Apr 13, 2018

Further successful flight testing in both nav and non-nav modes, tricopter, vtail and deadcat. I've now exhausted my stock of non-quadX machines, and they are all fine with this PR.

I will fly a quadX tomorrow --- it would be nice if someone was able to test it on FW.

@stronnag
Copy link
Collaborator

Flew te quadx today, as before no issues. So that's my complete iNav MR fleet, 2xtri, 1 quadx, 1 deadcat, 1vtail, 250mm- 600mm.

@DzikuVx
Copy link
Member Author

DzikuVx commented Apr 14, 2018

and that concludes testing I suppose. Thanks @stronnag

@stronnag
Copy link
Collaborator

I'm really happy with it on MR. FW with its range of servo diversity is something else. It would be good if a developer could flight test it on FW before it goes into an RC (imho).

@DzikuVx
Copy link
Member Author

DzikuVx commented Apr 14, 2018

I might fly it tomorrow on a flying wing.
The funny fact is that if it worked on a Tri, it will work everywhere else. Anyhow, just flashing my wing

@DzikuVx
Copy link
Member Author

DzikuVx commented Apr 15, 2018

Flight tested on a flying wing. No problems of any kind during flight

@digitalentity digitalentity added this to the 2.0 milestone Apr 21, 2018
@DzikuVx DzikuVx merged commit 9dfd2d0 into development Apr 21, 2018
@digitalentity digitalentity deleted the dzikuvx-mixer-rework branch April 21, 2018 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants