-
Notifications
You must be signed in to change notification settings - Fork 84
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
API support #98
Comments
@gelse which principle of RESTful API did the message API break? I meant it to be used with Home automation; what blocks you there? For MQTT, idk. While beeing a fan of MQTT, somehow the message usecase does not feel like being a bit benefit to me. What other control would you imagine? Creating new endponts is pretty simple... |
@kohlsalem well, that was not meant as a "it does break REST", it was meant as "it does not support full control". I really like to be able to switch plugin, set dimmer, etc. but ... well, if you are asking ... I guess, as "message" is changing the state of the target, it rather should be POST and not GET, but tbh, that's nitpicking and i dont care as long as it works. ;-) |
Hmm. I am failing currently with a proper implementation of a night mode. Moving this beater to homeassistent instead sounds like an interesting idea to me… and yes, you got me on the post, probably ;-) can you provide me with a list of all things you want to see?
|
I just noticed- I never checked the communication form the gui with the backend. Shouldn’t there be already something for plugins, brightness, rotation? |
this API is called by a machine, so "convenience" is not much of an issue, therefore i dont see much use of rotate left/right, set orientation makes more sense to me. The same goes for "next/prev plugin" - which is nice for a user, but does not make much sense in an api. I will think a bit about more things that would make sense tomorrow. |
i think that's solved via a websocket, which is not really useful for a restful api - i think. |
I created a PR, #99 some thoughts:
I did not have the chance to test it yet, so do not hit to hard if something obvious is wrong. |
yikes ... i would have used a POST, but as i learned some weeks ago, the better verb is even PATCH. |
ok, all 3 new endpoints seem to work fine. i just ... HAVE to make a full review. sorry for nitpicking here. thats the inner senior dev. ;-) the response on false values should be valid json, the response code should not be 404 but 422 and getbrightness and getplugin is propably missing, it's not really of much use, but it makes sense to have a getter for a setter. |
NP :-) While I appreciate the consideration for beauty and symmetry, our ESP32 is running low on memory, and I'd rather not expend it on unnecessary elements. |
yes, im fully aware of post. But then i (and everybody) have to se wget instead of a browser to test. Thats why i have it on get. The change would be obviously trivial, but... |
I misunderstood your answer, sorry. Well... Yes. That's the whole point of an API? It is not intended to be used with a browser. |
422 seems to be the broadly accepted value for such things. Anyway, 404 is just wrong, it's purpose is to indicate that the endpoint is not there, not that parameters are missing or wrong. But I see your point about the low memory and agree with you there. Still, if the target is a HomeAssistant Integration, something to get the current state is necessary. Perhaps it's better on the memory if a common GetState endpoint is implemented, which then could also include the list of available Plugins, thus making that specific endpoint obsolete? |
All possible. Thats how its implemented in the websocket implementaion, with a "big" json. Still i would somehow prefer something like "Get Status", returning brightness and active plugin... |
so, getstatus instead of "get plugin list" |
oh ok, i did not see you already started work on the code. anyway, i took the challenge and implemented it myself, the PR is here: #100
The memory used is still at 309k, not much more than without the separate endpoints. |
Some people mentioned it already and a rudimentary API is already implemented with the message and graph support, but i would really like to see a way to control it via any form of API.
For example a RESTful interface could replace the message endpoint and enable to use it with Home Automation software like HomeAssistant.
Another possibility, tho i see those as two different topics, would be to add MQTT support.
(sadly my knowledge and experience with C is not yet good enough to code it myself)
The text was updated successfully, but these errors were encountered: