-
Notifications
You must be signed in to change notification settings - Fork 101
Add JSON Api #196
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?
Add JSON Api #196
Conversation
|
The patch looks reasonable and I appreciate that it ticks many checkboxes (dependencies, docs, schema, changelog, separate cpp module, etc). However, for now it is best to keep these changes in a fork. Some thoughts:
Also I'm (slowly) working on CAT protocol support and an extended command set. I hope to make it transport independent, so it can be used over serial, bluetooth, as well as Wi-Fi. It could enable more remote control use-cases than just JSON. With that said, we clearly see the need to expand the web UI (more settings, separate radio control page, ability to edit memory slots, etc), and might cherry-pick some of your patches later on. Thanks! |
I'm glad we have the same concerns over code quality 😄
I completely understand the unease over adding a big dependency like NPM. However advancements on the CI/CD side of things greatly simplify handling such a dependency during the release process (the changes to also build a npm project are trivial to the workflow, see this for a possible example) while certain development techniques allow developers to completely ignore that part if not interested in it (for example in my fork if the npm project isn't built then the firmware build will still succeed omitting the web pages part, see this). This project lives in a rom/firmware context where space is a constraint and it would be very convenient to piggy back on all the minification efforts of the web world (they have a similiar constraints on the network side of things). I completely agree that settings are totally fine as a static HTML form but I don't believe all the c/ccp overhead required to generate an HTML element is good. When developing new features for the web interface a developer should focus on the functionality instead of wearing a "HTML generator" hat and mangling HTML strings in procedural code. #187 is an example of what I mean by developer complexity/code overhead, the intent was to add a new HTML element in a static HTML form. Fortunately this PR is about adding a JSON api not necessarily only usable by the status pages and we don't need to think about the above now 😅
I'm new to the CAT protocol you mention, are you referring to https://en.wikipedia.org/wiki/Computer_aided_transceiver? If so, wouldn't that protocol only enable remote control use-cases for software that speaks the specific protocol? My understanding is that JSON has become the de-facto standard in interoperability so it seems the less limiting choice. Couldn't the project support both and when the CAT interface is available work can be done to refactor the JSON code to only be an interface without duplicating logic code? Also the JSON interface would be ready now and other projects could start adding support for controlling the radio.
I hope I don't pass as polemical and I am very grateful you guys are maintaining this awesome project 🤩 I believe it is fantastic that I have the chance to participate in a community and discussing technical themes like the above brings a great feeling 🧐. Thank you for all the great work done so far. |
7ca30ca to
4924291
Compare
e888cb3 to
a936d86
Compare
…ve the same data the web server pages show.
…gin for rendering in docs.
These changes add a new JSON Api that exposes the same information that is currently available in the status and config web pages. The /api/config endpoint also allows setting the same settings available in the config web page.
The interface is documented via a dedicated docs page and OpenAPI v3 specification.
More info regarding these changes can be found in the discussion I started at #195