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

chores(flysky): separate AFHDS2A and AFHDS3 into 2 different module type #3783

Merged
merged 23 commits into from
Sep 7, 2023

Conversation

raphaelcoeffic
Copy link
Member

@raphaelcoeffic raphaelcoeffic commented Jul 9, 2023

Fixes #836

This should allow to remove some of the hacks added earlier.

@raphaelcoeffic raphaelcoeffic force-pushed the separate-afhds-protos branch 2 times, most recently from e4337b4 to b9b084c Compare July 10, 2023 06:03
@raphaelcoeffic
Copy link
Member Author

@philmoz @richardclli may I outsource separating the AFHDS2A / AFHDS3 options UI to you? I think this is mostly about splitting flysky_settings.{h,cpp} into afhds2a_settings.{h,cpp} and afhds3_settings.{h,cpp}, as this PR introduces 2 separate module types.

@elecpower may I also outsource the Companion changes to you? I believe the focus is probably on the YAML conversion here. The conversion is basically from type=TYPE_FLYSKY & subType=[AFHDS2A | AFHDS3] to type=[TYPE_FLYSKY_AFHDS2A | TYPE_FLYSKY_AFHDS3].

@raphaelcoeffic
Copy link
Member Author

@pfeerick should we add the el18 target to CI as well? I think we should have el18 replace nv14 in the tests, and have final builds for both (second phase).

@elecpower
Copy link
Collaborator

@elecpower may I also outsource the Companion changes to you? I believe the focus is probably on the YAML conversion here. The conversion is basically from type=TYPE_FLYSKY & subType=[AFHDS2A | AFHDS3] to type=[TYPE_FLYSKY_AFHDS2A | TYPE_FLYSKY_AFHDS3].

Accepted. After I finish merging pots and sliders config into pots config with all the necessary conversions for old and new settings which I am about half way thru fixing.

@pfeerick
Copy link
Member

@pfeerick should we add the el18 target to CI as well? I think we should have el18 replace nv14 in the tests, and have final builds for both (second phase).

Yup, sounds like a good time to do it... want me to add/swap the bits to build_gh and actions? This probably also makes life a bit easier with Companion as then we can start supporting the AFHDS2A and AFHDS3 specific options, which lines up nicely with my "want" for EL18 to get Companion support this cycle :)

@pfeerick
Copy link
Member

I think that's all the "generic" bits. I also made a slight change to the order of tests - as I've found the x9dp2019 is the most likely B&W radio to implode if something goes wrong (partly as it probably has the most stuff turned on / hence also the least free space), so moved it to the top so it either passes or fails quicker. TX16S is probably the most representational horizontal LCD, and then the nl14/el18 for portrait LCD... I don't really think we need to do both for the tests post this PR, but probably should do until it's finished, since the tests will break the builds quicker?

@elecpower
Copy link
Collaborator

Companion base work done. Will move on to testing NV14 by comparing with its libsim.
Unable to test EL18 libsim yet as has not been added to Companion that I can find.
There appears to be a need for some Companion clean up of AFHDS3 as it appears to be a hack of an old AFHDS2A. eg there is a binary import mapping which how it existed back then I do not know. In this case I'd suggest we just drop the import and any legacy fields in moduledata.h.

@pfeerick
Copy link
Member

pfeerick commented Jul 12, 2023

There is zero support for EL18 in Companion at present, hence my earlier comment. Working on #3247 is on my 2.10 wish-list 😁

Can 100% drop any bin import for EL18 as it is post yml, and I'm 99% sure of the top of my head NV14 is also, so it's probably something from the flysky fork of NV14 which we don't support migrating from anyway, so IMO it can all go bye bye and make life simplier.

@elecpower
Copy link
Collaborator

@pfeerick would you like me to start a PR for #3247 in the next few days/week? I'd put all the building blocks in place and leave it to you to refine/fix.
That would be enough for me to run up the libsim to help test this PR.

@elecpower elecpower added color Related generally to color LCD radios B&W Related generally to black and white LCD radios companion Related to the companion software and removed needs: companion labels Jul 12, 2023
@pfeerick
Copy link
Member

@pfeerick would you like me to start a PR for #3247 in the next few days/week?

Yes, that would be great. With a skeleton in place, I can run through it with the actual hardware and make/suggest/beg for any changes.

@elecpower
Copy link
Collaborator

What no grovel 😉

@pfeerick
Copy link
Member

beg

It was implied 🤣

@elecpower
Copy link
Collaborator

The nv14 libsim crashes in:

  • Radio Settings clicking hardware tab
  • Model Settings | Setup tab | clicking Int. Module

Does not crash in main branch so will wait for it to be fixed before mirroring the ui in Companion.

@raphaelcoeffic
Copy link
Member Author

@elecpower potentially this was caused by TR_MODULE_PROTOCOLS missing one element, which should be fixed with b091c27.

@elecpower
Copy link
Collaborator

@elecpower potentially this was caused by TR_MODULE_PROTOCOLS missing one element, which should be fixed with b091c27.

@raphaelcoeffic that fixed the crash, thanks

@elecpower
Copy link
Collaborator

Tested with #3807

@philmoz
Copy link
Collaborator

philmoz commented Jul 27, 2023

@pfeerick would you like me to start a PR for #3247 in the next few days/week? I'd put all the building blocks in place and leave it to you to refine/fix.
That would be enough for me to run up the libsim to help test this PR.

I started working on the firmware UI changes. NV14 working ok; but I'm a bit stuck without companion support for EL18.

@richardclli
Copy link
Collaborator

@philmoz @richardclli may I outsource separating the AFHDS2A / AFHDS3 options UI to you? I think this is mostly about splitting flysky_settings.{h,cpp} into afhds2a_settings.{h,cpp} and afhds3_settings.{h,cpp}, as this PR introduces 2 separate module types.

@elecpower may I also outsource the Companion changes to you? I believe the focus is probably on the YAML conversion here. The conversion is basically from type=TYPE_FLYSKY & subType=[AFHDS2A | AFHDS3] to type=[TYPE_FLYSKY_AFHDS2A | TYPE_FLYSKY_AFHDS3].

Let me take a look first, work on the same branch with you?

@philmoz
Copy link
Collaborator

philmoz commented Jul 27, 2023

PR #3861 separates the UI settings pages for AFHDS2A and AFHDS3.

@elecpower
Copy link
Collaborator

PR #3807 has this PR as its base for the EL18 simulator. So Companion side is done except for fix ups that @pfeerick was going to check when he had a spare minute lol

@philmoz
Copy link
Collaborator

philmoz commented Jul 27, 2023

PR #3807 has this PR as its base for the EL18 simulator. So Companion side is done except for fix ups that @pfeerick was going to check when he had a spare minute lol

Hmm, I can't see EL18 as a radio type in Companion for setting up to run the simulator.

Ok, so I need 3807 as well?

@pfeerick
Copy link
Member

pfeerick commented Sep 4, 2023

Let me know your thoughts on this one @philmoz :)

@pfeerick pfeerick marked this pull request as ready for review September 4, 2023 09:53
@philmoz
Copy link
Collaborator

philmoz commented Sep 4, 2023

Let me know your thoughts on this one

It seems ok to me. I've used this when building for EL18 and the radio binds and works with the AFHDS3 receiver.

Can't test the NV14 / AFHDS2A side though.

@pfeerick
Copy link
Member

pfeerick commented Sep 4, 2023

Thanks. I'll try the NV14 side out then, and just give the EL18 a look over.

@pfeerick
Copy link
Member

pfeerick commented Sep 7, 2023

NV14 seems to be working both prior to and post this PR with no major issues - receiver still working fine, settings read and wrote fine on radio, and settings also survived Companion sync unscathed. I was going to try the FRM303 with TX16S but it wouldn't cooperate on 2.9.0 (or this PR) so I need to see why the mod to support the module isn't working.

@pfeerick pfeerick added this to the 2.10 milestone Sep 7, 2023
@pfeerick pfeerick merged commit c4f1e71 into main Sep 7, 2023
40 checks passed
@pfeerick pfeerick deleted the separate-afhds-protos branch September 7, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B&W Related generally to black and white LCD radios color Related generally to color LCD radios companion Related to the companion software
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NV14 Internal Module protocol
5 participants