-
-
Notifications
You must be signed in to change notification settings - Fork 430
chore(radio): remove hardware definitions present in JSON #6437
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
base: main
Are you sure you want to change the base?
Conversation
d0a88e7 to
c858fca
Compare
fe4bf6a to
feebb79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a step backward to me. There is no reason to require a human entry for this as the navigation type is purely defined by the available keys.
Maybe I am misunderstanding; but I thought the idea was to remove the KEYS_GPIO_xxx stuff? |
|
This will be continued once #6095 has been merged. |
15200fc to
59cd5c9
Compare
|
Perhaps some cleanup for the commented out blocks in unless some of that is commented out debug code?
and taking the draft flag off if you feel this is ready to go (since you need it for other PRs to carry on ;) ) After #6392 has been merged? 🤭 Any specific handsets you need tested? Else I'll be using the X9D+2016, TX16S, TProV2, NB4+ to start with... |
I'm not quite sure what to with that: on one hand, we might need that code again for some special purposes (like generating stuff as a one-off), but on the other, this is not supposed to happen once the transition is made to JSON-only. I'd suggest cleaning up this code later on, once we are confident with the transition being completed. |
Regarding targets that have been started before this PR gets merged, the best course of action is to generate the JSON before rebasing on top of Regarding the PA01 in particular, I have no string feelings, either way before or after is fine with me. Worst case I'll generate the missing JSON out of |
|
Please merge after the PR i will submit today ;) |
|
Since the code is still present in the repo I wasn't so worried about
keeping "old code" but either way is fine by me. So is the draft flag
coming off as you're ready to live with the consequences, or is this PR
going to hide behind it a bit longer? 😂
Please merge after the PR i will submit today ;)
From the sounds of it Raphael is getting antsy about merging this PR
also... ok, he had a point... PRs going forward can be updated to include
the relevant `.json` so I suspect you're not going to get that wish... both
TX15 and PA01 are still only announced, not in the wild yet AFAIK
(pre-order doesn't count) so we have time to go with this, and can make him
fix anything that gets broken because of it 🤭
…On Tue, 29 July 2025, 7:56 pm 3djc, ***@***.***> wrote:
*3djc* left a comment (EdgeTX/edgetx#6437)
<#6437 (comment)>
Please merge after the PR i will submit today ;)
—
Reply to this email directly, view it on GitHub
<#6437 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJ66KNE5YPF7NQUFHOEQBT3K5AN3AVCNFSM6AAAAACBL3D5XSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCMZRGY2TGOJRGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Depends, there will be quite a lot to test. I need to re-check with my scripts that we don't have any usage of the removed defines. Also verify that the tooling is effective and allows us to potentially add more defines to the list of removed stuff later on (whereby this is not a huge priority, we can see then).
Really, I have no problem rebasing on top of these 2 PRs. The real question for me is: will be able to stop building simulator plugins for F2 radios once we have the JSON for these radios "frozen" and shipped with Companion without building anything? @elecpower what do you say? |
|
Okey dokey, just wanted to be sure of the status of this PR, as it's holding up you being able to continue with #6435. Better to get the other two in since they're only days away at most and give this time for testing. But I don't think it should be held back for too long... after all, best testing is usually having to have to use the darn thing, and yank it if there are major issues. AFAIK it should address the Cpn issue but Neil is the proper authority there ;) |
|
Companion loads all json files it finds at compile time so provided devs don't decide to clean up the frozen unsupported radios we should be okay in the short term. However once the json definition changes the frozen files will become less usable/reliable. What I would like to see is the inclusion of a definition version in all json files. That makes it clear to Companion how to interpret and at what point they become unsupported for conversion. |
We can enforce conformity to the data model. This is the purpose of the validator I added. At the moment, it is not yet running in "strict" mode (would fail on additional attributes not defined in the data model), but we could make it run that way and add it to the GH checks.
I would prefer to keep it simple. Given that we have schema checks, changing the schema also forces us to update the JSON for legacy radios, or remove them altogether. I hope this is sufficient. |
|
I can live with keeping unsupported radio json files conforming for a reasonable time. We keep all Companion releases so there is a conversion path should a user wait too long to transfer their models. |
# Conflicts: # tools/generate-hw-defs.sh
Switches can now have a `pin` defined without a `gpio`.
…style. Remove unused TLiteF4 variant.
c897609 to
d3f3d25
Compare
As we now have JSON files for some parts of the hardware definitions, it would be
good to remove these redundant definitions and use only the JSON files.
For a smooth transition, the removal and checks should happen as automated as possible.
See
radio/util/hw_defs/README.mdfor more details.