ph1p/ikea-led-obegraensad

API support

Opened this issue · 16 comments

gelse commented

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)

kohlsalem commented

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

gelse commented

@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.
And when the api is implemented, it could be merged with the current /message endpoint.

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

kohlsalem commented

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?

  • Activate Plugin (with id)
  • Next Plugin
  • Previous Plugin
  • set Brightness
  • Nightmode on/off (switch off and stay). Or may be Display on/off
  • Rotate left/right
  • set Timezone?! (This would be a different PR)
  • ??? Anything else
kohlsalem commented

I just noticed- I never checked the communication form the gui with the backend. Shouldn’t there be already something for plugins, brightness, rotation?

gelse commented
  • get plugin list (id and name)
  • set plugin by id
  • set brightness (parameter: 0-255)
    • 0 can be seen as manual nightmode
  • set nightmode duration (parameter: n minutes)
    • would switch to brightness 0 for n minutes
    • sounds to me like a separate PR, as that functionality is not in yet at all
    • same goes for an automated night mode by time - if we have brightness exposed via API, the external tool (like homeassistant) should take care of the timing.
  • set orientation (parameter 0-3)
  • some way to send parameters to plugins if they need some, but i guess that would also need a separate refactoring

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.
But in the api you definitely need the list of the available plugins.

I will think a bit about more things that would make sense tomorrow.

  • 👍1
gelse commented

I just noticed- I never checked the communication form the gui with the backend. Shouldn’t there be already something for plugins, brightness, rotation?

i think that's solved via a websocket, which is not really useful for a restful api - i think.
but the endpoints should be internally available already i think

  • 👍1
kohlsalem commented

I created a PR, #99
not tested yet, but feel free to.

some thoughts:

  • No night mode yet - why would'nt i just set the brightness to 0?
  • No orientation yet. I can not imagine why i would ever set the orientation after initial setup.
  • No Plugin Parameters - Plugins could easily register there own endpoints. "AsyncWebServer server;" is external, so i see no restriction.

I did not have the chance to test it yet, so do not hit to hard if something obvious is wrong.

gelse commented

To set an active plugin by ID, make an HTTP GET request to the following endpoint:
http://your-server/setplugin

yikes ...
that (and setbrightness) really should not be a GET...
a GET never should have any effect on the server.

i would have used a POST, but as i learned some weeks ago, the better verb is even PATCH.

gelse commented

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.

kohlsalem commented

NP :-)
422 - Unprocessable Entity
404 - Not Found
I initially lean towards 404 for its logic, but if you prefer 422, that works for me. So does the reasoning for JSON,

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.

kohlsalem commented

To set an active plugin by ID, make an HTTP GET request to the following endpoint:
http://your-server/setplugin

yikes ... that (and setbrightness) really should not be a GET... a GET never should have any effect on the server.

i would have used a POST, but as i learned some weeks ago, the better verb is even PATCH.

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

gelse commented

But wget actually supports POST? (And curl would be better anyway - also there are really easy tools like insomnia, or my favourite: Bruno)

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.

gelse commented

NP :-)
422 - Unprocessable Entity
404 - Not Found
I initially lean towards 404 for its logic, but if you prefer 422, that works for me. So does the reasoning for JSON,

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.

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?

kohlsalem commented

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

  • 👍1
kohlsalem commented

so, getstatus instead of "get plugin list"

gelse commented

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
for my tastes, my code improved yours a bit:

  • removed data from getStatus and made a separate endpoint which makes usage of the AsyncResponseStream.
  • cleaned up the response messages (no need to send "OK" if the status is 200)
  • added a tiny bit of error handling and prettier error messages
  • moved the implementation of the endpoints to the messages file

The memory used is still at 309k, not much more than without the separate endpoints.