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

chore: refactor "Flysky gimbal" to "Serial gimbal" #5713

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rotorman
Copy link
Member

No description provided.

@3djc
Copy link
Collaborator

3djc commented Dec 16, 2024

The issue I can see is there might be other serial gimbal protocols looming around..

@rotorman
Copy link
Member Author

rotorman commented Dec 16, 2024

As the same proto was used by also other companies as FS, the name seemed not appropriate anymore.
Would you have a better suggestion how to work around this?

@pfeerick
Copy link
Member

As the same proto was used by also other companies as FS, the name seemed not appropriate anymore.

IMO... yes and no... if the same protocol is being used... and Flysky came up with it first... then it is still Flysky serial protocol... but I think perhaps it is more that new radios, i.e. GX12 is using serial gimbals, not necessarily Flysky protocol, so instead a new specifier is needed to cover that case, or at least a generic specifier. i.e. perhaps "flysky serial gimbal" is a subtype of "serial gimbal" ;)

@pfeerick pfeerick added the house keeping 🧹 Cleanup of code and house keeping label Dec 16, 2024
@3djc
Copy link
Collaborator

3djc commented Dec 17, 2024

As the same proto was used by also other companies as FS, the name seemed not appropriate anymore.

IMO... yes and no... if the same protocol is being used... and Flysky came up with it first... then it is still Flysky serial protocol... but I think perhaps it is more that new radios, i.e. GX12 is using serial gimbals, not necessarily Flysky protocol, so instead a new specifier is needed to cover that case, or at least a generic specifier. i.e. perhaps "flysky serial gimbal" is a subtype of "serial gimbal" ;)

GX12 is using flysky serial gimbal protocol, I was thinking about another manufacturer which serial gimbal will use a completely different protocol. I'm personally not too fussed that the first to bring up a feature/standard/code gets his name for it, we call CRSF the radio to module ELRS protocol too

@richardclli
Copy link
Collaborator

I have concern about this. Flysky has a new version of protocol which supports sync sampling. And I am working on it, and I cannot guarantee that this change is compatible with other serial gimbals. I think the best way is to split the implementations.

@3djc
Copy link
Collaborator

3djc commented Dec 18, 2024

That's a possibility, but then how do we select which one to use ? Ok, Flysky radio get FS one, RM rm; Jumper jumper, but what does it mean for say FrSky radio that wants to add one of those 3. Do we include none and provide a compilation option for user to build their own custom firmware ?

@rotorman
Copy link
Member Author

rotorman commented Dec 18, 2024

Also, what if non-FS radio is modded with FS gimbals, like is possible: https://github.com/EdgeTX/edgetx/wiki/Flysky-Hall-Sticks-Mod

Ideally, our code should auto-detect the gimbal attached.

@3djc
Copy link
Collaborator

3djc commented Dec 18, 2024

Ideally I would agree, but I don't think any of those protocols actually allows that

@rotorman
Copy link
Member Author

The present FS implementation has a checksum, which could be one way to validate if present non-sync FS protocol is being used, or not.

@3djc
Copy link
Collaborator

3djc commented Dec 18, 2024

RM serial gimbal use 100% the same code, but for synchronisation where they use a dedicated line, so at this point in time, I have forced it on GX12 to assume it is a RM gimbal, so it uses Flysky serial code, and forces RM synchronization

@richardclli
Copy link
Collaborator

And my opinion is not necessary to do this renaming at this moment. RM just made a Flysky protocol compatible gimbal with sync enhancement. Not quite original to worth any refactoring effort just for a name!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
house keeping 🧹 Cleanup of code and house keeping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants