Ce sujet concerne la faculté de programmer un allumage progressif d’une ampoule Hue, comme présenté dans cette issue
Je n’ai pas eu beaucoup de temps à passer sur ce sujet, mais j’ai une première ébauche qui est fonctionnelle.
Je pense qu’elle nécessite discussion avant de poursuivre et de finir tout ce qui est nécessaire pour que la PR soit valide.
Les sujets qui portent à discussion :
Il n’y a pas encore dans le projet la notion de “scénario” associée à une Scène. Jusqu’à présent toute scène est composée d’une suite d’actions basiques comme allumer une lumière ou actionner un interrupteur. Faire un fade-in demande d’enchaîner plusieurs actions de bases, l’API Hue ne proposant pas d’action directe pour faire un allumage progressif. Ici on a donc besoin de chaîner plusieurs actions simples : allumer la lumière puis changer la luminosité à intervalles réguliers. Ce “scénario” demande un jeu de paramètres plus évolué que pour les actions basiques : on a besoin à la fois de la durée de l’allumage et de la luminosité cible. Ce n’est pas possible d’intégrer ces paramètres dans le modèle actuel et j’ai dû tricher en modifiant la validation du format des actions d’une scène pour avoir quelque chose qui tourne. Je pense donc qu’il faut réfléchir à quelque chose d’adapté.
Côté front, j’ai encore dû tricher pour le Component LightFadeInParams pour gérer cette “action” avec plusieurs paramètres. A discuter aussi pour faire quelque chose de plus propre.
J’ai joué avec des setTimeout pour faire l’allumage progressif mais je n’aime pas trop cette solution : on risque d’avoir un allumage qui finit en retard. Il faudrait sûrement s’aider d’une librairie plus adaptée. S’il n’y a pas de package adapté déjà utilisé dans Gladys, je regarderais ce qui pourrait faire l’affaire. De plus, quand le temps d’allumage est court, l’API Hue semble changer l’état de la lumière un peu en retard par rapport au retour d’appel. Ce qui provoque des “saccades” dans le processus d’allumage progressif. Je vais regarder pour peut-être limiter le nombre d’appels à l’API.
Il y a pas mal d’autres choses à revoir sur la PR mais je préfère discuter de ces sujets avant d’aller plus loin.
ce n’est pas « tricher », c’est comme ça qu’on développe à chaque fois ! Si tu as besoin de plusieurs attributs dans une action, c’est normal de rajouter ces attributs à la validation.
Tu pourrais montrer directement le code pour montrer en quoi c’est « tricher » ?
Haha effectivement je ne me suis attardé que sur l’API sur le site Philips et n’ai pas été assez curieux pour regarder ce que proposait le module node directement. My bad, je vais regarder pour l’utiliser.
Pour préciser mon propos sur où j’ai modifié la validation du modèle : lien
Voilà où j’ai triché dans le front, dans l’implémentation du formulaire de création d’action de fade in : lien
Cela est lié à la validation du modèle d’une scène, comme j’ai ajouté un attribut “parameters” pour gérer l’ensemble de mes valeurs (voir le lien précédent), je mets à jour l’attribut “parameters” en entier à chaque fois plutôt que de ne mettre à jour qu’un sous-attribut de “parameters”. C’est pas hyper clean, mais en l’état actuel je ne vois pas comment faire autrement.
Ah effectivement c’est pas fou, pourquoi tu n’as pas fais 2 variables ? pas besoin d’une variable parente « parameters ».
On le fait dans plusieurs actions de scène pourtant.
Exemple dans ton cas:
handleChangeTargetBrightness = e => {
this.props.updateActionProperty(this.props.columnIndex, this.props.index, 'target_brightness', e.target.value);
};
D’ailleurs, je remarque que tu fais un setState en plus de ton updateActionProperty, c’est pas terrible car ça t’oblige à synchroniser les 2 états. Tu pourrais directement utiliser la props props.action qui est injecté par le parent !
Si j’utilise trois variables, je dois modifier l’actionSchema du modèle scene et ajouter trois attributs target_brightness, duration_value et duration_unit. Ces attributs ne seront utilisés que pour ce type d’action. D’où l’idée d’utiliser un unique attribut parameters de type object pour rendre le schéma générique et réutilisable bien que très permissif.
C’est ce qu’on veut éviter ! Dans Gladys 4, on fait tout au plus stricte. Si le schéma est trop souple, on va se retrouver avec n’importe quoi en DB, on va avoir des utilisateurs avec des problèmes mystiques, et ça sera très dur à débugger et à corriger.
Là ou je suis d’accord avec toi, c’est que ce schema qui valide de la même manière toutes les actions c’est bizarre et pas assez stricte.
Je pensais pas que c’était possible avec Joi au début, mais en fait on peut faire des schémas conditionnels avec .when() ( joi.dev - 17.12.0 API Reference )
J’aimerais bien migrer vers ça pour qu’on puisse valider individuellement chaque type d’action.
Voilà pourquoi je voulais en discuter, je n’aime pas l’idée d’avoir des options qui seront null la plupart du temps et je pense aussi que parameters est trop permissif.
Je vais regarder les schémas conditionnels.
Salut,
Après une hibernation de 4 mois j’ai repassé un peu de temps sur la PR.
Je pense qu’elle est prête pour une review, j’ai construit une image pour mon Rasp 4 et ça tourne bien.
@VonOx si tu souhaites tester.
Merci @pierre-gilles et @AlexTrovato pour la première relecture, j’ai pris en compte les retours.
Déjà merci pour ta PR et tout le travail réalisé dessus, c’est cool
J’ai écris une review sur ta PR.
Ta PR fait des changements assez majeure sur l’API à pas mal d’endroits qui ne sont pas lié au Philips Hue et je pense que ça vaudrait le coup d’en discuter avant, et si ces changements sont vraiment nécessaires (je ne suis pas sûr), je pense que ça a plutôt sa place dans une PR séparé.
On essaie de faire des PR plutôt petites en général, et quand il y a des changements d’API majeure (comme ici dans ta PR) avec des migrations, c’est quelque chose dont on discute en général avant.
Je pense que c’est super important d’avoir une API stable et non breaking dans le temps, car des utilisateurs en dépendent, et codent potentiellement des scripts, des intégrations externe qui en dépendent, donc ce n’est pas à prendre à la légère ce genre de changements.
J’avais abordé plus haut dans cette conversation pourquoi j’avais besoin de faire ces modifications.
J’ai besoin de trois paramètres pour configurer mon action : l’intensité cible, l’unité de temps et la valeur pour la durée de l’action.
L’objet action n’était pas adapté selon moi :
Les seules valeurs possibles étaient text, value et unit ne donnant aucune information sur la nature réelle du contenu de la variable.
Ces valeurs se trouvent au même niveau que type et devices, on n’a pas de distinction entre les paramètres qui définissent la nature d’une action et ceux qui permettent à une action d’un certain type de tourner.
En revanche je n’avais pas pris en compte le fait que l’API peut être consommée par d’autres consommateurs que le front de Gladys. Je pense qu’il faut effectivement que je revienne sur les modifications de l’interface de l’API, mais je propose de conserver la migration du modèle de données pour les raisons juste au dessus.
L’introduction d’une notion de scenario permet de définir des actions plus complexes que simplement activer une feature. Je ne l’ai pas appelé fadeIn parce que la notion de scenario pourrait être utilisée pour n’importe quel type de device, le fadeIn par contre est spécifique aux ampoules. Après, je l’ai appelé scenario mais ça m’est égal d’utiliser un autre wording
L’objet action est tout fait extensible à souhait: tu mets ce que tu veux dedans
Si tu veux rajouter 3 attributs libre à toi!
On avait discuté avant de rajouter des check spécifique à chaque action (au lieu de faire un check global), pas de créer un objet nested dans l’objet action.
Pour moi, on gagnerait en lisibilité à garder un objet action « flat », pas besoin de créer des niveau supplémentaires.
On a déjà une fonctionnalité de scénario dans Gladys donc ça porte à confusion vu que ça n’a pas de rapport ici.
Si tu veux faire de l’ultra générique, pourquoi ne pas passer par le setValue dans ce cas là?
Une des difficultés que j’ai rencontrée pour rentrer dans le concept de scènes et actions est justement ce manque de hiérarchie dans les objets action. Je trouve qu’un objet flat rend compliqué la compréhension sur le rôle de chaque propriété.
Ma compréhension du setValue est le passage d’une feature d’un état à un autre pour un device donné. L’appel déclenche le changement d’état et retourne directement. Cela permet de répondre à un appel d’API dans un temps raisonnable.
Ici, l’action fait partie d’une scène qui peut être composée de plusieurs groupes d’actions qui doivent s’exécuter séquentiellement.
Imaginons une scène où je veux que mon ampoule commence à s’allumer à 7h20 pendant 10 minutes et qu’ensuite ma radio s’allume. Avec un setValue qui retourne directement, ma radio s’allumerait à 7h20. J’ai donc besoin d’attendre que l’état de mon ampoule soit l’état final désiré pour mettre fin à mon action et déclencher la suivante.
De plus, dans mon “scenario” je fais appel à deux features différentes de l’objet light : binary et brightness. Le setValue devrait rester sur une seule feature.
Je pense que si jamais à l’avenir quelqu’un code un petit tool pour faire des scènes en YAML et les importer dans Gladys, ça sera plus lisible.
Après bon c’est un peu les goûts et les couleurs, je pense c’est aussi bien de garder quelque chose de léger, sans trop d’objets à instancier, et pas trop nested ^^
Dans l’API oui, après dans une scène tu peux faire un setValue qui attend que le device ait répondu, tout est possible
Je pensais que tu passais par l’API de Philips Hue qui fait ça en une commande ?
Ne prend pas mal mes remarques hein, je trouve ça super d’être challengé sur les choix fait à différents endroits
Mais je défend aussi les choix de format de donnée choisi, ça n’a pas été choisi à la légère et je pense que c’est important de rester rétro-compatible dans la mesure du possible (à part si vraiment c’est méga bloquant et qu’on veut avancer).
Je pense que c’est un bon signal à envoyer à la communauté que l’API est “stable” et ne change pas dans des versions mineurs de Gladys tous les 4 matins
J’ai besoin d’allumer la lampe et de réinitialiser l’intensité au minimum au début de l’action avant d’utiliser la commande de changement d’intensité progressif. Je change donc plusieurs états sur des features différentes d’un même device.
