Sonoff service (over MQTT) #519
Conversation
|
@Pierre-Gilles J'ai un problème de conception sur les "sous-services" MQTT. Peut-être faut-il créer un service Sonoff, un service Owntracks... utilisant le service MQTT ? Un point de réflexion reste ouvert. |
|
@atrovato Je comprend le problème! Partons de l'expérience utilisateur, pas de la tech Côté front, ça fait sens d'avoir une interface dédié "Owntracks", et une interface dédié "Sonoff". Si je suis utilisateur, et que j'ai des périphériques Sonoff, j'ai envie de cliquer sur une icône "Sonoff" et d'avoir un bloc qui explique comment configurer tout ça. Pareil pour Owntracks: je suis utilisateur Owntracks, je m'attend à voir une intégration "Owntracks" dans l'interface, on est d'accord ? On ne doit pas présupposer que l'utilisateur sache comment ça marche sous le capot, du moins pas avant de lui avoir expliqué dans une intégration dédié au périphérique. Côté back, à voir si on sépare ou non. Deux solutions pour moi:
Exemple: Côté MQTT on expose une fonction "listen(topic, callback)", et côté service Sonoff on appelle cette fonction pour chaque topic qu'on écoute. Chaque solution a ses avantages et ses inconvénients. Personnellement je pencherais plus pour la solution n°2, ça évite que le service MQTT deviennent énorme. Dis moi ce que tu en penses :) |
|
On dira que le point flou que je vois encore, c'est la gestion du start / stop du service MQTT par le sous-service Sonoff... Il faudra un stop "partiel" du service Sonoff (dans le cas où plusieurs protocoles seront gérés). |
|
Il faudrait un moyen de retirer un listener. On pourrait avoir deux fonctions "addListener(topic, callback)" et "removeListener(topic)". Après bon, pour certains services, le start/stop à moins de sens je trouve |
atrovato
force-pushed
the
sonoff-mqtt-service
branch
from
ff1c8d6
to
da1633b
Compare
|
Based on #517 PR Le service MQTT a été refactoré, afin de pouvoir y "connecter" d'autres services, dont celui-ci. |
atrovato
force-pushed
the
sonoff-mqtt-service
branch
from
f3dcd44
to
d6de27a
Compare
atrovato
force-pushed
the
sonoff-mqtt-service
branch
from
82119de
to
ccc9129
Compare
|
@Pierre-Gilles si tu as un peu de temps, peux-tu faire une review ? |
There was a problem hiding this comment.
Hey! Thanks for this great PR :)
On the code side, it's really good, I had a look and it seems all the best practices are here, good job 👏
I have a few UX feedbacks. I have absolutely no knowledge in how Sonoff works, so I was a good candidate to discover this integration with a fresh eye! Here are my feedbacks:
When I arrive, everything is empty and I have no clue what's the difference between "Discover" and "Devices". How do peripherals appears in Discover? When I create a device in "Devices", what is the goal ? Why topics ? So it works with MQTT? So I have to configure the MQTT service first? What if MQTT is not configured yet? Am I able to configure this service even if MQTT is not working?
-> You should add more message explaining how things work :)
-> Maybe detect the configuration of the MQTT service ?
( btw, the question above are real question I'm asking myself :p )
-
A little explanation on what is a topic, rules, naming should be nice.
-
I'm not able to modify the "topic" of a device, because you use the "topic" as the "external_id". I'm not sure this is the best place to save the topic. The external_id is meant to be something static that never changes and identify uniquely a device. Here it seems it's not the case. Maybe it should be saved elsewhere? Let's talk about this!
When I create a device with the same name as another device, I have a generic error message.
-> This is because you don't provide a selector, and use an auto-generated one.
-> Should it be a good idea to have 2 devices with the same name in your integration? If yes, please provide a more unique selector, if no, please explain why this error happens.
-> In general, we should have a clear message based on the 409 error. I think we should create a generic error component to be able to handle HTTP errors everywhere the same way in the frontend, especially for devices :) (this is a more general feedback for Gladys 4 front)
The description is a little long compared to other one, it doesn't look good on my screen. Is the second part of the desc necessary?
This is my first feedback! Again, good job for this service, well done 👏
|
First, thank you for you review, and your great approach as "not aware of Sonoff workflow". As I know how it works, it was clear enough for me, but your questions were very constuctive.
I will not answer here, but I expect you will find answers in UX improvements.
On device tab :
On discover tab :
This point needed to change the already merged MQTT service and add a new "check status" API entry point and UI component.
The topic is a Sonoff (Tasmota) key word that can be found on Sonoff device UI. Please see
As the Topic is the unique device identifier on MQTT, I suppose that external_id was the most appropriate. Maybe we can disable the fact to modify its value and force user to recreate a new one if he changes its device own configuration. Let's talk about this 😉
I managed the 409 case.
Description cutted 😃 I add 'sonoff:' prefix in external_id value from manual device creation. And, as I just discover the |
atrovato
force-pushed
the
sonoff-mqtt-service
branch
from
089cb0d
to
4ae8562
Compare
|
The build is not successful, but I think there is an issue with CircleCI. |
|
This is circle ci limitation on free plan 😣 ( 1000 minutes of build) And 250/week https://circleci.com/changelog#free-minutes-shift-to-250-week |
|
@atrovato Seems CircleCI is limiting more strictly the time we can build per week. As an open-source project, we should be able to have additional build time. ( https://circleci.com/blog/building-open-source-projects-on-circleci/ ) I just sent a message to support to ask for free credit as open-source project, I'll let you know when they answer me! |
|
"Hi Pierregilles, Thanks for contacting us. We'll be happy to help you with this. An account representative will be in touch soon. Jun Ishioka |
There was a problem hiding this comment.
This is my review on the UX side (waiting for CircleCI on the tech side)
-
Good job for the MQTT status route, good for me 👍
-
Good for me for the warning message for MQTT
-
On the UX side, it's way better than before but I still don't understand how to configure Sonoff devices automatically if I buy some. So they are publishing MQTT messages by default? Why can I add them manually or automatically? Why not everything automatically? Should I flash my devices with a new firmware before? There needs to be a clear configuration step by step explanation when clicking on the integration. It can be a link to the external documentation if it's really too long to explain in Gladys.
-
You need to add the MQTT status route to the demo.json file.
-
Still one question about MQTT topics, are MQTT topics forced by Sonoff or can you use anything? It would be great if MQTT topics could be more in the naming convention of Gladys 4 ( see the MQTT documentation )
-
You need to update your demo example with
sonoff:external_id ! -
I don't see the change on the selector for unique names
-
I'm ok with not allowing users to change the MQTT topic, but before let me know about my question on automatic vs manual add (question 3)
|
Sonoff devices need to be flashed with the Sonoff-Tasmota firmware, according to the tutorial link in the discovery page.
They send a "discovery message". But they need to be configured first, according to the Sonoff-Tasmota link.
I first develop the manual creation workflow, to prepare the arrival on new devices.
To prepare Gladys before connecting it to MQTT broker, before geeting all devices...
Yes, and it's not so easy to do... that's why I linked to the official firmware website.
I don't think it's a good idea to copy / paste the official Sonoff-Tasmota tutorial. There is a lot a compatible devices, and for each, there is a way to flash it. The official website should be enough, but it's only in english.
Done.
There is no way to change MQTT message format. Only a part of the MQTT topic is overridable, which defines the device identifier.
Done.
I did it.
Ok to disable edition on topic (refers external_id), but it means that if I change the topic (refers device identifier) on the device, I will have to delete the existing device in Gladys and create (or discover) a new one. So I think the main open question is about the documentation (how to flash device ? how to configure it ?). I'm always listening for constructive remarks 😉 |
|
I will change the "tutorial" link to add a link to Gladys documentation, and in the document page, sowe could add the official link, and more specific Galdys documentation about Sonoff. |
Ok, this should be explicit in the UI! How is the user supposed to know this? :)
I'm good with that! Let's link first to Gladys doc, then redirect when needed to different tutorials :) FYI, I have a call with CircleCI today at 6:30pm to unlock this payment issue |
|
At 6:30 pm french time ? I will also add the "device.setValue" method, as it will come soon with Philips Hue service :) |
|
Yes french time!
nice! |
|
Follow up on CircleCI call: Good news: they should unlock us soon, and we shouldn't have to pay 15$/Github user/month (because it's an open-source project) Can't wait to build & publish the work done here :) |
|
Please review also documentation PR at https://github.com/GladysAssistant/gladys-4-docs/pull/2 |
atrovato
force-pushed
the
sonoff-mqtt-service
branch
from
02ef311
to
96a520a
Compare
Codecov Report
@@ Coverage Diff @@
## master #519 +/- ##
==========================================
+ Coverage 91.78% 91.92% +0.14%
==========================================
Files 383 396 +13
Lines 4700 4819 +119
==========================================
+ Hits 4314 4430 +116
- Misses 386 389 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey! Just tested all this.
It's great, but I saw several little things to fix. After that I think we are good for a merge :)
I have a recurring bug in the UI: I create 2 devices manually, then I cannot access anymore the "devices" tab.
Here are the two devices:
[{"id":"f9d61926-d42b-4e07-83b0-453852ec3f9d","service_id":"34c8c938-82b2-4dfb-8c0c-5d666d1ce017","room_id":"017a5f80-aa33-4e58-967a-37f6f733e0a2","name":"My device","selector":"sonoff-test-test","model":"sonoff-pow","external_id":"sonoff:test-test","should_poll":false,"poll_frequency":null,"created_at":"2019-10-24T11:34:50.924Z","updated_at":"2019-10-24T11:34:56.872Z","features":[{"id":"b240e50e-768d-4c0a-97fd-e9ff33079c02","device_id":"f9d61926-d42b-4e07-83b0-453852ec3f9d","name":"My device","selector":"sonoff-test-test-switch-binary","external_id":"sonoff:test-test:switch:binary","category":"switch","type":"binary","read_only":false,"keep_history":true,"has_feedback":true,"unit":null,"min":0,"max":1,"last_value":null,"last_value_string":null,"last_value_changed":null,"created_at":"2019-10-24T11:34:50.936Z","updated_at":"2019-10-24T11:34:56.889Z"},{"id":"7c925be2-1966-40e7-b69d-2aab6d6940a9","device_id":"f9d61926-d42b-4e07-83b0-453852ec3f9d","name":"My device - power","selector":"sonoff-test-test-switch-power","external_id":"sonoff:test-test:switch:power","category":"switch","type":"power","read_only":true,"keep_history":true,"has_feedback":false,"unit":"A","min":0,"max":10000,"last_value":null,"last_value_string":null,"last_value_changed":null,"created_at":"2019-10-24T11:34:56.881Z","updated_at":"2019-10-24T11:34:56.881Z"}],"params":[],"room":{"id":"017a5f80-aa33-4e58-967a-37f6f733e0a2","house_id":"0d0394e0-89dc-4921-a2c6-9bc8730ac11f","name":"room","selector":"room","created_at":"2019-10-18T14:42:55.167Z","updated_at":"2019-10-18T14:42:55.167Z"},"service":{"id":"34c8c938-82b2-4dfb-8c0c-5d666d1ce017","pod_id":null,"name":"sonoff","selector":"sonoff","version":"0.1.0","enabled":true,"has_message_feature":false,"created_at":"2019-10-24T11:19:46.943Z","updated_at":"2019-10-24T11:19:46.943Z"}},{"id":"5ff35f8b-8f1e-4053-963b-68b32fcdf8bd","service_id":"34c8c938-82b2-4dfb-8c0c-5d666d1ce017","room_id":"017a5f80-aa33-4e58-967a-37f6f733e0a2","name":"My device","selector":"sonoff-other-device","model":"sonoff-basic","external_id":"sonoff:other-device","should_poll":false,"poll_frequency":null,"created_at":"2019-10-24T11:37:10.473Z","updated_at":"2019-10-24T11:37:12.437Z","features":[{"id":"b572977d-2ca8-4ff2-b833-3aa0da184b9f","device_id":"5ff35f8b-8f1e-4053-963b-68b32fcdf8bd","name":"My device","selector":"sonoff-other-device-switch-binary","external_id":"sonoff:other-device:switch:binary","category":"switch","type":"binary","read_only":false,"keep_history":true,"has_feedback":true,"unit":null,"min":0,"max":1,"last_value":null,"last_value_string":null,"last_value_changed":null,"created_at":"2019-10-24T11:37:10.479Z","updated_at":"2019-10-24T11:37:12.444Z"}],"params":[],"room":{"id":"017a5f80-aa33-4e58-967a-37f6f733e0a2","house_id":"0d0394e0-89dc-4921-a2c6-9bc8730ac11f","name":"room","selector":"room","created_at":"2019-10-18T14:42:55.167Z","updated_at":"2019-10-18T14:42:55.167Z"},"service":{"id":"34c8c938-82b2-4dfb-8c0c-5d666d1ce017","pod_id":null,"name":"sonoff","selector":"sonoff","version":"0.1.0","enabled":true,"has_message_feature":false,"created_at":"2019-10-24T11:19:46.943Z","updated_at":"2019-10-24T11:19:46.943Z"}}]
|
Wait, the My bad! :) So there are just the translations problem to fix. |
* MQTT: sonoff (front/back) + owntracks * MQTT: device creation by subService * sonoff : scan for devices * Sonoff: restructure MQTT service * Sonoff: try to fix import * Sonoff: improve model name * Sonoff: complete demo mode * Sonoff service: Fix typo * Sonoff service: refactor models * Run prettier :/ * Sonoff service: fix front impacts * Sonoff service: fix model name * Sonoff: limit integration description * Sonoff: improve discover UX messages * Sonoff: improve devices UX * MQTT service status * Sonoff: use device.model iso params * Sonoff: use device.model iso params * MQTT: fix status UX behavior * Sonoff: improve error message * Sonoff: topic help * Sonoff: pushing to re-run circleCI * Sonoff: update demo.json according to changes * Mqtt/Sonoff: changing service client for device * Sonoff: add ability to change value * Sonoff: force 'sonoff:' topic prefix * Sonoff: improve manual/auto device creation * Sonoff: documentation link * Sonoff: fix selector + clean * Sonoff: forgot to adapt tests * Sonoff: test coverage * Sonoff: documentation link * Sonoff: fix typos
Pull Request check-list
To ensure your Pull Request can be accepted as fast as possible, make sure to review and check all of these items:
npm teston both front/server)npm run eslinton both front/server)npm run prettieron both front/server)front/src/config/demo.json) so that the demo website is working without a backend? (if needed) See https://demo.gladysassistant.com.NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
Completes the MQTT service with Sonoff management according to V3 modules.