(v3) Home Assistant notification support #971

dbuezas · 2023-12-02T11:59:36Z

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 · 2023-12-02T12:34:33Z

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 <ESP8266HTTPClient.h> 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 · 2023-12-02T13:06:01Z

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 · 2023-12-02T20:59:09Z

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 · 2023-12-02T21:12:58Z

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 · 2023-12-02T21:44:29Z

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 · 2023-12-02T22:56:10Z

Pending:

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: "\ "
~~Rebase to right branch~~
-> stay in this one
Change max len of T1 in ESP3D-UI
Document usage in readme

luc-github · 2023-12-03T00:02:01Z

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 · 2023-12-03T00:15:24Z

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 · 2023-12-03T00:18:51Z

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 · 2023-12-03T07:51:07Z

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 · 2023-12-03T07:58:08Z

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 · 2023-12-03T08:02:10Z

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 · 2023-12-03T08:04:34Z

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

luc-github · 2023-12-03T08:19:46Z

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 · 2023-12-03T08:21:32Z

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 · 2023-12-03T09:15:15Z

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 · 2023-12-03T09:23:45Z

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 · 2023-12-03T09:42:39Z

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.

* Add notification via post request * Extend t1 size to 255 bytes * Correct all memory references

dbuezas · 2023-12-03T09:52:31Z

Allright, anything else pending apart from these two then?

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

luc-github · 2023-12-03T10:34:49Z

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 · 2023-12-03T11:31:32Z

So Can I merge ?

dbuezas · 2023-12-03T12:28:16Z

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

luc-github · 2023-12-03T12:29:01Z

like for the T1 size ^_^

dbuezas · 2023-12-03T12:32:35Z

esp3d/src/core/commands/ESP400.cpp

@@ -97,9 +97,9 @@ const char* FirmwareLabels[] = {"Unknown", "Grbl", "Marlin", "Smoothieware",
 const char* FirmwareValues[] = {"0", "10", "20", "40", "50"};
 #ifdef NOTIFICATION_FEATURE
 const char* NotificationsLabels[] = {"none", "pushover", "email",
-                                     "line", "telegram", "ifttt"};
+                                     "line", "telegram", "ifttt", "home-assistant"};


Ok for this one to have a dash?

yes I see no issue

dbuezas · 2023-12-03T12:33:00Z

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

luc-github · 2023-12-03T12:37:03Z

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 · 2023-12-03T13:06:56Z

✅ Done.

Btw MAX_NOTIFICATION_TOKEN_LENGTH doesn't seem to be used anywhere.

luc-github · 2023-12-03T13:10:04Z

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 · 2023-12-03T13:10:52Z

Merged thank you

* Remove all output flags * Masse replace name function / class to sync with ESP3D-TFT * Create ESP3DMessageManager class to handle messages creation / deletion (import functions of ESp3DClient from ESp3D-TFT) * Move to new messaging API of ESP3D-TFT * Remove empty line from remote screen dispatching * Replace \n to space and \r to nothing in remote screen dispatching * Add missing default entry for sensor device * Fix buzzer for ESP32 missing ledc initialization with latest core * Move formatBytes to esp3d_string * Add welcome message to telnet connection\ * Add display to the new messaging API * Add sticky authentication on Serial / SerialBridge /Telnet/Data web socket and BT * Log simplification * Use enum class instead of typdef enum for ESP3DAuthenticationLevel to be sure correct enum is used * (v3) Home Assistant notification support (#971) * Add notification via post request * Extend t1 size to 255 bytes * Hide Home Assistant notifications from web UI (#974) * Sync with ESP3DLib on serial_socket * Add some sanity check to avoid unnecessary message copies * Update ESP0.cpp --------- Co-authored-by: David Buezas <dbuezas@users.noreply.github.com>

dbuezas changed the title ~~Home Assistant notification support~~ (v3) Home Assistant notification support Dec 2, 2023

dbuezas force-pushed the dbuezas/HA-support branch from 33992de to 2070b76 Compare December 2, 2023 20:49

dbuezas and others added 7 commits December 3, 2023 10:44

Home assistant support WIP.

0ef6037

* Add notification via post request * Extend t1 size to 255 bytes * Correct all memory references

Update setting version

fa010ff

Reduce T1 to 250 bytes

b847611

Update esp3d_settings.cpp

9a559ce

update settings version

61e29b1

Comment out debug log

227c195

Template Wait4Answer

74a9689

dbuezas force-pushed the dbuezas/HA-support branch from 5aa9c70 to 74a9689 Compare December 3, 2023 09:46

dbuezas mentioned this pull request Dec 3, 2023

Home Assistant notification docs luc-github/esp3d.io#32

Merged

dbuezas mentioned this pull request Dec 3, 2023

Longer T1 token for Home Assistant notifications luc-github/ESP3D-WEBUI#373

Merged

Clean up

c57e2bf

dbuezas commented Dec 3, 2023

View reviewed changes

Fix missing token length corrections

f5522b1

luc-github merged commit c7957a2 into luc-github:Refactoring-internal-clients Dec 3, 2023

dbuezas mentioned this pull request Dec 3, 2023

(v2) Add Home Assistant notification support #968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(v3) Home Assistant notification support #971

(v3) Home Assistant notification support #971

dbuezas commented Dec 2, 2023

luc-github commented Dec 2, 2023 •

edited

Loading

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023 •

edited

Loading

luc-github commented Dec 3, 2023

luc-github commented Dec 3, 2023 •

edited

Loading

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023 •

edited

Loading

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023 •

edited

Loading

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023 •

edited

Loading

luc-github commented Dec 3, 2023

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023

dbuezas Dec 3, 2023

luc-github Dec 3, 2023

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023

luc-github commented Dec 3, 2023

(v3) Home Assistant notification support #971

(v3) Home Assistant notification support #971

Conversation

dbuezas commented Dec 2, 2023

luc-github commented Dec 2, 2023 • edited Loading

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023

dbuezas commented Dec 2, 2023 • edited Loading

luc-github commented Dec 3, 2023

luc-github commented Dec 3, 2023 • edited Loading

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023 • edited Loading

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023 • edited Loading

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

dbuezas commented Dec 3, 2023 • edited Loading

luc-github commented Dec 3, 2023

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023

dbuezas Dec 3, 2023

Choose a reason for hiding this comment

luc-github Dec 3, 2023

Choose a reason for hiding this comment

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023

dbuezas commented Dec 3, 2023

luc-github commented Dec 3, 2023

luc-github commented Dec 3, 2023

luc-github commented Dec 2, 2023 •

edited

Loading

dbuezas commented Dec 2, 2023 •

edited

Loading

luc-github commented Dec 3, 2023 •

edited

Loading

luc-github commented Dec 3, 2023 •

edited

Loading

dbuezas commented Dec 3, 2023 •

edited

Loading

dbuezas commented Dec 3, 2023 •

edited

Loading