homieiot / homie-esp8266

💡 ESP8266 framework for Homie, a lightweight MQTT convention for the IoT
http://homieiot.github.io/homie-esp8266
MIT License
1.36k stars 308 forks source link

advertiseRange is missing #701

Open kartom opened 3 years ago

kartom commented 3 years ago

In version 3 the function advertiseRange(const char* property, uint16_t lower, uint16_t upper) is missing.

This might be on purpose, since the functionality can be achieved in other ways, but then the parameter range should be removed from all input handlers and the documentation updated.

As of today, the range is detected from the property name if it contains n underscore, the last part of the name is the range index. This is however unfortunate for two reasons:

  1. Underscores are not valid as property id's according to the Homie standard: "ID MAY ONLY contain lowercase letters from a to z, numbers from 0 to 9 as well as the hyphen character (-)"
  2. Users who are unaware about this undocumented feature might end up with stange behavior that can be hard to track down,

Since the function advertiseRange was removed for two years ago and nobody reported it before it can't be a particularly widely used function. Therefore a removal of all functionality regarding ranges might be the best solution and instead include a nice example of how this could be achieved in user code. This would also have the nice effect of shrinking the code size.

kartom commented 3 years ago

It seam like arrays (or ranges as it is named in the ESP826 implementation) was moved from properties to nodes so for a node you can define an array, but the documentation tells you nothing about it.

This is the documentation from the API: HomieNode(const char* id, const char* type, std::function<bool(const String& property, const HomieRange& range, const String& value)> handler = ); the correct line would be: HomieNode(const char* id, const char* name, const char* type, bool range, uint16_t lower, uint16_t upper, <bool(const HomieRange& range, const String& property, const String& value)> handler); And this line comes from the Advanced usage/Input handlers: HomieNode node("id", "type", nodeInputHandler); whilst the correct code would be: HomieNode node("id", "type", false,0,0 nodeInputHandler);

So the only thing needed seams to be updating the documentation (however, the arrays seams to be removed in the version 4 of the Homie standard?)

kartom commented 3 years ago

More errors in the documentation ... In the Advanced usage/Input handlers the line bool nodeInputHandler(const String& property, const HomieRange& range, const String& value) should be bool nodeInputHandler(const HomieRange& range, const String& property, const String& value).

Does anybody actually use this? Has the code been tested and does it work?

kartom commented 3 years ago

I can confirm that the code seams to be working, it's only the documentation that needs to be updated.

luebbe commented 3 years ago

I never used ranges. Since you already figured it out, would you be so kind to create a PR for the necessary doc changes?

kartom commented 3 years ago

I have made a pull request on all updates i have spotted. However i do think that the documentation around ranges needs further improvements to be understandable. Its also a bit confusing that its called arrays in the covention document and ranges in the implementation.

luebbe commented 3 years ago

Does #703 close this issue?