Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

advertiseRange is missing #701

Open
kartom opened this issue Jan 3, 2021 · 6 comments
Open

advertiseRange is missing #701

kartom opened this issue Jan 3, 2021 · 6 comments

Comments

@kartom
Copy link
Contributor

kartom commented Jan 3, 2021

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
Copy link
Contributor Author

kartom commented Jan 3, 2021

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
Copy link
Contributor Author

kartom commented Jan 3, 2021

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
Copy link
Contributor Author

kartom commented Jan 3, 2021

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

@luebbe
Copy link
Collaborator

luebbe commented Jan 3, 2021

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
Copy link
Contributor Author

kartom commented Jan 4, 2021

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
Copy link
Collaborator

luebbe commented Jan 11, 2021

Does #703 close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants