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

Version as string? #306

Open
nstadigs opened this issue Nov 15, 2024 · 12 comments
Open

Version as string? #306

nstadigs opened this issue Nov 15, 2024 · 12 comments

Comments

@nstadigs
Copy link

I'm currently using a hash of the description to generate the version. But I just realized that this is wrong according to the specification. It should be a number.

I've found that using a hash as version is deterministic, makes testing easier and simplifies reinitialization of devices.

@schaze
Copy link
Contributor

schaze commented Nov 15, 2024

I also use a hash. But I use a SipHash 1-3 which computes a 64bit key (8 bytes) that I convert to a number and use as version.
@Tieske: Can you remember why we used a number for the version instead of a string?

@Tieske
Copy link
Contributor

Tieske commented Nov 16, 2024

iirc to increment it. Hashing is not really viable on small devices, and certainly not reliable with JSON (binary equality vs functional equality).

imho using a number is generally easier. On small devices comparing numbers is easier.

I use a timestamp in seconds from the date I wrote the code.

@schaze
Copy link
Contributor

schaze commented Nov 17, 2024

Thanks @Tieske!
Here my thoughs:
Using a hash on a JSON is not reliable I agree. But using a hash on the native data structure (not JSON) in my code that is used to generate the JSON is. So for me hashing is a viable option - not sure how @nstadigs is doing it. Luckily the hash algorithm available to me hashes into a 64bit result so I can still represent it as a number.

However using a string for a version would allow for more flexibility:

  • if you want to use a number simply convert it back and forth from string to increase it. If you use timestamps just also convert them to string and set them as version - no need to read in the current one. A string to number conversion and vice versa must IMHO be possible on all devices otherwise you will not really be able to implement a mqtt based homie device.
  • if you want to use a hash that results in some kind of base64 or other alphanumerical hash result you can use this with a string field as well

For the controller side of the homie device the only real check to be done is if the version changed, not really if it got bigger or smaller. For this usecase it is also not important if version is a number or a string.

@nstadigs
Copy link
Author

nstadigs commented Nov 17, 2024 via email

@nstadigs
Copy link
Author

I can actually get an integer out of fnv1a instead of hexes, so this isn't really an issue for me anymore, but the discussion about dropping the constraint may still be still relevant

@Tieske
Copy link
Contributor

Tieske commented Nov 17, 2024

A string value makes sense. The only requirement is that it changes.

I would be in favour of limiting length btw.

@nstadigs
Copy link
Author

I agree. Limited length makes sense imo.

@schaze
Copy link
Contributor

schaze commented Nov 17, 2024

I concur. What length limit should we choose? 24,32,64 characters?

@Tieske
Copy link
Contributor

Tieske commented Nov 18, 2024

It can be way less than that. Sha-256 has 64 chars. But we don't need that for uniqueness, eg. Git uses 7 chars for the short id.

@nstadigs
Copy link
Author

The version is only for the controller to know if the description has changed from one time to the next, right? If that's the case then even if the collision rate of your hashing algorithm is relatively high the chances of your controller getting the same version as the last visit (even tho the description has changed) seems very low. I think you can just use the fastest and shortest you can find and you'll probably be fine

@Tieske
Copy link
Contributor

Tieske commented Nov 18, 2024

the technicalities are easiest solved, I think we agree there. So can you raise a PR, to validate the exact wording?

@schaze
Copy link
Contributor

schaze commented Nov 18, 2024

Just one more comment about limiting the length: is it really necessary? Technically I haven't seen a mqtt client that would be able to limit the size of a message it receives. So you can limit what you send manually from a library but what do you do with a too long version string that you receive?
So in the end it may just be a more or less useless restriction.
IMHO just defining it is to be a string is enough. What difference does it make if it is 12 or 120 characters long?

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

No branches or pull requests

3 participants