edgetx icon indicating copy to clipboard operation
edgetx copied to clipboard

chore(radio): remove hardware definitions present in JSON

Open raphaelcoeffic opened this issue 5 months ago • 12 comments

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.md for more details.

raphaelcoeffic avatar Jul 12 '25 15:07 raphaelcoeffic

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?

philmoz avatar Jul 17 '25 06:07 philmoz

This will be continued once https://github.com/EdgeTX/edgetx/pull/6095 has been merged.

raphaelcoeffic avatar Jul 25 '25 04:07 raphaelcoeffic

Perhaps some cleanup for the commented out blocks in unless some of that is commented out debug code?

  • cmake/Macros.cmake
  • radio/src/bootloader/CMakeLists.txt
  • radio/util/hw_defs/hal_json.py

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...

pfeerick avatar Jul 28 '25 23:07 pfeerick

Perhaps some cleanup for the commented out blocks

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.

raphaelcoeffic avatar Jul 29 '25 09:07 raphaelcoeffic

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 https://github.com/EdgeTX/edgetx/pull/6392 has been merged?

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 main with this PR already merged. This way, the JSON generated can be added to the target's PR.

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 main, and add it to this PR. That's not a big issue.

raphaelcoeffic avatar Jul 29 '25 09:07 raphaelcoeffic

Please merge after the PR i will submit today ;)

3djc avatar Jul 29 '25 09:07 3djc

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) https://github.com/EdgeTX/edgetx/pull/6437#issuecomment-3131653916

Please merge after the PR i will submit today ;)

— Reply to this email directly, view it on GitHub https://github.com/EdgeTX/edgetx/pull/6437#issuecomment-3131653916, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABJ66KNE5YPF7NQUFHOEQBT3K5AN3AVCNFSM6AAAAACBL3D5XSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTCMZRGY2TGOJRGY . You are receiving this because you commented.Message ID: @.***>

pfeerick avatar Jul 29 '25 21:07 pfeerick

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?

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).

both TX15 and PA01 are still only announced, not in the wild yet AFAIK

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?

raphaelcoeffic avatar Jul 30 '25 08:07 raphaelcoeffic

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 ;)

pfeerick avatar Jul 30 '25 08:07 pfeerick

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.

elecpower avatar Jul 30 '25 09:07 elecpower

However once the json definition changes the frozen files will become less usable/reliable.

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.

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.

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.

raphaelcoeffic avatar Jul 30 '25 09:07 raphaelcoeffic

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.

elecpower avatar Jul 30 '25 10:07 elecpower