(v3) Home Assistant notification support

[dbuezas]

To avoid double work, I'll ask here before I start implementation for v3.

• Is it ok if I make the PR against this branch? (Refactoring-internal-clients)
• Ok if I make token1 255 bytes long?
• I failed to make this work with the existing ssl client (like the other notifications) in v2
  • let me know if there is a trick or
  • if it is ok to #include <ESP8266HTTPClient.h> specifically for this. I can put it behind a flag to make it optional.
• I'll avoid reusing the email settings parser this time and pass config like this:
  • M118 [ESP610]type=HOME_ASSISTANT TS=homeassistant.local:8123 T1=access-token
  • Alternatively, T1 could be used to pass arbitrary headers (e.g Authorization: Bearer YOUR-TOKEN)
  • M118 [ESP600]/api/services/switch/toggle#{"entity_id": "switch.3d_printer_switch"}
• Since slicers confuse brackets for variables, maybe it would be wiser to offer a char replacement to avoid them, suggestions?
• Do you prefer me referring to this notification as HOME_ASSISTANT or POST_REQUEST?
  • I suggest POST_REQUEST and having Home Assistant as an example.
  • This may lead to other usages and maybe others extending other request settings via PR
  • It opens the possibility for GET_REQUEST support down the line.

[luc-github]

Is it ok if I make the PR against this branch? (Refactoring-internal-clients)

No better use the current 3.0 , this branch will be under heavy testing in coming weeks so will bring mess for you to work on branch that is not stable

Ok if I make token1 255 bytes long?

Yes no issue for V3 but need to update validity function and position in EEPROM

I failed to make this work with the existing ssl client (like the other notifications) in v2 let me know if there is a trick or if it is ok to #include specifically for this.

I am not sure to understand your issue the file already has this include: https://github.com/luc-github/ESP3D/blob/3.0/esp3d/src/modules/notifications/notifications_service.cpp#L44

Since slicers confuse brackets for variables, maybe it would be wiser to offer a char replacement to avoid them, suggestions?

No sorry, I have no plan and no willing for that - the parsing is made to be as simple as possible to not overload ESP, giving most time to major task, adding an alternative char means double the workload of ESP for this, when issue is another software cannot handle the char - prusa slicer had same issue and they fixed it - better fix the issue at the root instead of trying to workaround in ESP3D, or next time the alternative char will be also an issue with a new slicer, etc... - it will never end ...

Do you prefer me referring to this notification as HOME_ASSISTANT or POST_REQUEST?

HOMME_ASSISTANT is ok I think, POST_REQUEST won't be clear for people , 99% people do not read docs orz..

It opens the possibility for GET_REQUEST support down the line.

GET is already available with [ESP620], open for all requests in one API is a small project by itself, need some time to list all possible and necessary parameters (http / https, headers, parameters, workflow, etc...) and some reflexion to limit the code footprint, currently I have no bandwidth for such task... but you are welcome to try ^_^ So just add HOME_ASSISTANT workflow is the best mitigation for the moment, better to do a little and doing properly than trying to do everything and failed everywhere IMHO

[dbuezas]

Makes sense. Re ESP8266HTTPClient.h, I didn't realize it was already in use, nevermind. I'll retest the v3 branch, maybe I had power supply issues with the usb<->uart board

[dbuezas]

OK, I didn't rebase to the original v3 branch yet, there are a lot of conflicts of course, but I want to get it working first.

It already works if I hard code the auth token as in the other MR, but even though I changed all memory indexes to make space for the extra bytes in T1, I'm still getting trash when I try to read it. Even crashes some times, so it is obviously writing in the wrong place.

Any tip on what I may have missed?

[dbuezas]

The token is parsed correctly

---- Sent utf8 encoded message: "[ESP610]type=HOMEASSISTANT T1=TOKEN TS=homeassistant.local:8123" ----
[ESP3D-ERROR][ESP610.cpp:83] ESP610(): CMD: type=
[ESP3D-ERROR][ESP610.cpp:84] ESP610(): tmpstr: HOMEASSISTANT
[ESP3D-ERROR][ESP610.cpp:83] ESP610(): CMD: T1=
[ESP3D-ERROR][ESP610.cpp:84] ESP610(): tmpstr: TOKEN
[ESP3D-ERROR][ESP610.cpp:83] ESP610(): CMD: T2=
[ESP3D-ERROR][ESP610.cpp:84] ESP610(): tmpstr: 
[ESP3D-ERROR][ESP610.cpp:83] ESP610(): CMD: TS=
[ESP3D-ERROR][ESP610.cpp:84] ESP610(): tmpstr: homeassistant.local:8123
ok

But then when reading it, it reads some nonsense (see beind Authorization: Bearer)

---- Sent utf8 encoded message: "[ESP600]/api/services/light/toggle#{\"entity_id\":\"light.wintergarten_spots\"}" ----
[ESP3D-ERROR][notifications_service.cpp:548] sendHomeAssistantMSG(): Query: POST /api/services/light/toggle  HTTP/1.1
Host: homeassistant.local
Connection: close
Cache-Control: no-cache
User-Agent: ESP3D
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Authorization: Bearer�(ESP3D-WebServer/1.0 (ESP82XX; Firmware/3.0.0.a225; Platform/arduino; Embedded; http://www.esp3d.io)
Content-Type: application/json
Content-Length: 40

{"entity_id":"light.wintergarten_spots"}

[dbuezas]

Oh wow, I think I found the issue. Reducing T1 to 250 bytes made it work. I had the hunch that some counter is 8 bit long and that results in either storing or reading T1 was getting out of whack. I'll test this in my printer for some days now.

[dbuezas]

Pending:

• [x] Figure out how to handle empty spaces in things like M118 P3 [ESP600]/api/services/tts/cloud_say#. {"entity_id":"media_player.vlc_telnet","message":"3d print ready"} -> escape with slash: "\ "
• [x] Rebase to right branch -> stay in this one
• [ ] Change max len of T1 in ESP3D-UI
• [ ] Document usage in readme

[luc-github]

Why did you changed :

bool NotificationsService::Wait4Answer(WiFiClientSecure & client,
                                       const char* linetrigger,
                                       const char* expected_answer,
                                       uint32_t timeout);

WiFiClientSecure Notificationclient;

to

bool NotificationsService::Wait4Answer(WiFiClient& client,
                                       const char* linetrigger,
                                       const char* expected_answer,
                                       uint32_t timeout);

 WiFiClient Notificationclient; 

This will impact others notifications that use https - your local server does not use https ?

[luc-github]

about the size yes - it seems I have in writeString / readString / isValidString and may be elsewhere: uint8_t size_max = query->size; I will fix that today

About embedded\config\server.js do not bother to change values of positions, they are not used

About doc it is actually landing: here: https://esp3d.io/esp3d/v3.x/documentation/commands/esp610/index.html here: https://esp3d.io/esp3d/v3.x/documentation/notifications/index.html and here https://esp3d.io/esp3d/v3.x/documentation/update/index.html

[luc-github]

no need to rebase to 3.0 it will make you crazy to manage conflict because the branch you use is a rewrite and nothing will match

[dbuezas]

Why change Wait4Answer

WiFiClientSecure inherits from WiFiClient, with this change it can be used with both.

Local home assistant uses http, and somehow I couldn't get WiFiClientSecure to work with the Internet facing https url either

[dbuezas]

no need to rebase to 3.0 it will make you crazy to manage conflict

Ok that's a relief 👍 And then you'd have to do the same work again to rebase your working branch.

[dbuezas]

About embedded\config\server.js do not bother to change values of positions, they are not used

Ok, good to know. It's done already so I may as well leave it there now.

Updating the ram positions was a bit of a pain, I reformatted the #define lines a bit so I could automate it.

[dbuezas]

Is there any way to make esp600's message contain spaces? That used to work in v2 and it's quite limiting

[luc-github]

Why change Wait4Answer

WiFiClientSecure inherits from WiFiClient, with this change it can be used with both.

Hmm no, Secure client use different API to read/write data: here for esp32 : https://github.com/espressif/arduino-esp32/blob/master/libraries/WiFiClientSecure/src/WiFiClientSecure.cpp#L232 I did not dig for esp8266

Did you tested that other notif still work ? I doubt it will

Using implicit casting is bad idea and not good habit because it can lead to nightmare to find issue root cause

[luc-github]

Is there any way to make esp600's message contain spaces? That used to work in v2 and it's quite limiting

yes of course : just add \ in front of every space https://esp3d.io/esp3d/v3.x/documentation/commands/index.html#conventions

[dbuezas]

Hmm no, Secure client use different API to read/write data: here for esp32

Wait, what? class WiFiClientSecure : public WiFiClient { Doesn't this mean that WiFiClientSecure inherits from WiFiClient and therefore whenever Wait4Answer calls an overridden function, the correct one will be called? As you see c++ is not my main language. I'm reading now about dynamic dispatching and the virtual keyword.

Should I make a duplicate of Wait4Answer for non-secure WiFiClients? What would be the best approach here?

Thanks for taking the time to go through the PR :)

Did you tested that other notif still work ? I doubt it will

I did not

[luc-github]

The class base is same because they have several methods in common but not read / write because ClientSecure use new ones as I linked to you. so calling WiFiClient won't call WiFiClientSecure methode

Yes I suggest to duplicate it to avoid issue

As I wrote before :

open for all requests in one API is a small project by itself, need some time to list all possible and necessary parameters (http / https, headers, parameters, workflow, etc...) and some reflexion to limit the code footprint, currently I have no bandwidth for such task... but you are welcome to try ^_^

[dbuezas]

Yes I suggest to duplicate it to avoid issue

Got it. To avoid duplicating the function, I made it a template. Let me know if that's not a good idea.

[dbuezas]

Allright, anything else pending apart from these two then?

• [x] Change max len of T1 in ESP3D-UI -> https://github.com/luc-github/ESP3D-WEBUI/tree/3.0
• [x] Document usage in readme -> https://github.com/luc-github/esp3d.io

[luc-github]

if you do not use it you should remove the code instead of comment it :

  //Notificationclient.setInsecure();
#if defined(ARDUINO_ARCH_ESP8266)
  //BearSSLSetup(Notificationclient);
#endif  // ARDUINO_ARCH_ESP8266

[luc-github]

So Can I merge ?

[dbuezas]

I think I'm done, yes. Let me take a last look at all the changes to be sure.

[luc-github]

like for the T1 size ^_^

[dbuezas]

I tested it again and it still works, so ok to be merged. Thank you!

[luc-github]

https://github.com/dbuezas/ESP3D/blob/dbuezas/HA-support/esp3d/src/core/esp3d_settings.cpp#L1252 => ok https://github.com/dbuezas/ESP3D/blob/dbuezas/HA-support/esp3d/src/core/commands/ESP400.cpp#L406 => still 63 https://github.com/dbuezas/ESP3D/blob/dbuezas/HA-support/esp3d/src/core/esp3d_settings.cpp#L68C1-L68C41 => still 63

Will you update ?

[dbuezas]

✅ Done.

Btw MAX_NOTIFICATION_TOKEN_LENGTH doesn't seem to be used anywhere.

[luc-github]

Ho

✅ Done.

Btw MAX_NOTIFICATION_TOKEN_LENGTH doesn't seem to be used anywhere.

Ho I missed this one when rewriting the setting API, so still may need more consolidation later then, because now have 3 differents sizes for token and they are hard coded orz...

[luc-github]

Merged thank you