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

Optional system description #95

Closed
wants to merge 1 commit into from

Conversation

davidgraeff
Copy link
Member

@davidgraeff davidgraeff commented May 2, 2018

Useful for OTA updates

Make IP, MAC optional

An MQTT client that only does MQTT communication does not need to provide it's IP and MAC. If a device is behind a proxy, the IP or MAC is not even of any use.

Introduce a $system topic

$system is just an example. $implementation can do it as well.
I have grouped all version, app, os, hardware information on this topic. That doesn't need to be the case.

I also have removed $fw/name and $fw/version. This is very specific for the case that a custom embedded firmware is used. That is not always the case, thing of a homie device node running on a raspberry pi.

Useful for OTA updates
@timpur
Copy link
Contributor

timpur commented May 2, 2018

honestly i think most of those props should be left under $implementation. I do like the idea though. Not so much the $system bit since you now have $system as device prop which sounds a bit odd to me since the device is the system ??

See the problem is there is a lot of complexity that implementations might add on eg http://marvinroger.github.io/homie-esp8266/docs/2.0.0/others/homie-implementation-specifics/. Where do we draw the line on what the standard defines and doesn't define.

If we do want to define some of these recommended things like ota, can we create a separate spec doc (new .md) so that the core doesn't become bloated with recommendations and optional bits. Lets keep the main doc about core and have other recommendations docs for eg if you going to implement ota, see this optional modules ???

@davidgraeff
Copy link
Member Author

Yes, modularisation of the spec into multiple documents would help a lot. This one document is too long already. But this is not part of this PR and I don't want to split and alter the original document in one PR.

@timpur
Copy link
Contributor

timpur commented May 2, 2018

lets talk more about this, but fist lets talk about the modules of the doc maybe? @ThomDietrich @marvinroger

@davidgraeff
Copy link
Member Author

@timpur A "homie device" can also be an app running on a linux host. It doesn't need to be "$system", but the name doesn't really matter, as you can see with the "device" example.

I just can't come up with a better name. Under the namespace some hardware information as well as operating system/application information need to be published. "system" matched somehow.

@timpur
Copy link
Contributor

timpur commented May 2, 2018

Whats wrong with where the existing stuff is now and $implementation? why does it have to be all under the one place $system? It made sense that a device has an ip thus its under device->$ip. What your saying is a device has a system which has an ip, device->$system->$ip.

Id just like to know why you think it all has to be grouped how you've proposed it? why break version compatibility? what are we gaining from this layout?

As i said i do think its worth discussing so dont take this as me turn the idea down.

@ThomDietrich
Copy link
Collaborator

ThomDietrich commented May 2, 2018

I'm also unclear about the decision against $implementation. It is perfect as it does cover both hardware and software aspects without implying it, which would be unfitting in the "app running on a linux host" scenario.

Regarding the document: I don't think it is too long. In fact I believe the fact that you can freely browse and Ctrl+F search all details at one glance is important.
Of course we could outsource additional parts like the FAQ at some point. I'd not do that in the near future.
I'd also not start breaking up required and optional properties just yet. We need to keep them together to give a clear view on the "whole package". A reader browsing the convention will be able to grasp what is important to him, what is not. We can't predict which properties are meaningful to the user. Optional or not, its value depends on the use case.

@davidgraeff
Copy link
Member Author

@TimPut. The IP/MAC is only required for OTA and other direct communication means. The homie spec should not anticipate that a device wants to be reached directly. And the spec didn't consider multiple assigned IPs for IPv6. The IP is assigned by the operating system/firmware, not the hardware, so it makes sense to have it under system.

@davidgraeff
Copy link
Member Author

davidgraeff commented May 2, 2018

$implementation is fine for me instead of $system. I removed the $implementation/# part though, because you can always add nodes on every topic, there is no need to mention this with an extra row.

IP and MAC are optional attributes, probably got introduced for OTA reasons, I guess. I would move them to the topic that tells about the firmware version/os name/architecture to have everything together, but that doesn't need to be the case.

@timpur
Copy link
Contributor

timpur commented May 2, 2018

Actually @davidgraeff the way OTA is implemented in the ESP8266 the IP is not needed for OTA, It is done over MQTT so... Your assumptions about the use of IP and MAC are limited, some people use them to access there own web service that they add on top of Homie for ESP8266.... again, we have to keep in mind how the user base uses Homie and how we can make that experience better for them.

The IP is assigned by the operating system/firmware, not the hardware, so it makes sense to have it under system.

Then basically everything should really come under system .... ?? Though you are right i dont think this means we have to put IP under System. Were not really gaining much from moving it to $system apart from making a breaking change.

You do make a good point about multiple IP's, so lets add that???

The homie spec should not anticipate that a device wants to be reached directly.

But it should anticipate OTA? This sounds like a contradiction.

@davidgraeff
Copy link
Member Author

apart from making a breaking change.

It is a breaking change anyway if the IP and MAC is made optional and/or if multiple IPs are allowed in the IP field.

@timpur
Copy link
Contributor

timpur commented May 2, 2018

@davidgraeff
How ? Optional doesn't break anything and IP can take CSV format doesn't breaker anything?

@davidgraeff
Copy link
Member Author

Of course multiple IPs do violate the spec. A spec-compliant parser right now can expect one ip address. It will fail to parse the field if a comma and even more characters are present. Changing a fields semantic is a breaking change.

@timpur
Copy link
Contributor

timpur commented May 2, 2018

@davidgraeff
Could be wrong here, but I don't see it as a breaking change since existing versions will still be supported by this change and don't break, only this change won't be supported by older versions, which you'd expect, to me this is just an enhancement then ?

To me a breaking change is only when older versions are no longer supported by new version ?

@davidgraeff
Copy link
Member Author

Hm, I thing you will understand with an example :)

Version Device Controller
Homie 3.0 homie/a/ip="1.1.1.1" SingleIPparser("1.1.1.1") -> 1.1.1.1
Homie 3.1 homie/a/ip="1.1.1.1,2.2.2.2" SingleIPparser("1.1.1.1,2.2.2.2") -> DOOM

@timpur
Copy link
Contributor

timpur commented May 2, 2018

Yes 3.1 passer will need updating, ofcause its a version change .... But 3.1 passer will still take 3.0 so 3.1 has not broken support for 3.0 thus not a breaking change to older versions ?

To support a new version, changes are aways needed, this is how enhancements work ? But a breaking change means the new change doesn't support the older versions ?

@davidgraeff
Copy link
Member Author

davidgraeff commented May 2, 2018

Homie follows SemVer according to the spec. A breaking change is NOT to be expected if it is not a major version change. Only extending is allowed. I'm on the controllers side of the view of course. But I expect my implementation for ESH to work until Homie 4.0. If you say, you want to break compatibility every minor version, the ESH controller will soon not work anymore.

@timpur
Copy link
Contributor

timpur commented May 3, 2018

@davidgraeff, you have a point and you are right for discovery platforms. Not sure what to do here, because it doesn't break the device but the discovery platform.

If we do follow that a breaking change is something that alos breaks discovery, then everything will be breaking... I think the device is our concern in this scope....

EDIT:
i had some more thought about it and there are some enhancements that can be made with out breaking device or discovery implementation, though limited. Question to you guys, what does homie consider a breaking change, when the device is no longer supported or both the device and the discovery implementation?

Something as simple as adding an additional required attribute will break older devices trying to be discovered by newer discovery implementations.... as an example.

@Thalhammer @marvinroger @euphi @ingoogni @fermuch @davidgraeff

@Thalhammer
Copy link
Member

Well, we already have $homie as a version identifier, so a controller can select an individual parser for each device. According to semver we should bump the major version if we break the API, minor if we add things without breaking and patch if we change things without new functionality/breaking.
Following this schema, a controller supporting for example version 3 should support every device with a major version of 3 (3.1, 3.2, ...) without changes to controller code. It might not be able to use features introduced in a later version (for example 3.1) but it should not break. However, a controller supporting version 3 is not required to be compatible with any device using version 2 or 4.

I did not fully read the whole issue, but here are my thoughts on this:

  • Implementation should be left as it is, no defined topics. It has been defined as a free format "put whatever you want" topic, so adding rules is almost guaranteed to break things.
  • IP Addresses are a problematic topic, there might even be devices that have none (custom transport, unix stream, etc). The same applies to mac addresses.
  • Multiple IPs could be supported without breaking by adding a device attribute "$secondaryip", but I doubt we even need a second IP since IP is intended as a way to connect to the device and you don't need multiple IP's for this.

We can add required topic's in minor versions or change one optional topics to be required, since older controllers will just ignore them. However the inverse is not possible since a controller supporting version 3 might expect a topic to exist and fail if it does not because it was made optional in version 3.1.

Maybe we should add additional feature subsets which are optional to be implemented (for example updates via mqtt). We could add a attribute that lists those specifications. This would allow us to keep the main document small and concise and still add new things without a problem.

@ThomDietrich
Copy link
Collaborator

@davidgraeff could you add a new commit so we can all see the current state of the discussion?
I'm not sure if my arguments in #95 (comment) are on your radar and what you think about them.

@davidgraeff
Copy link
Member Author

@Thalhammer Thanks for your thoughts. An optional feature subset is exactly what this PR is about. The probably planned location attribute (#3, #78) is an optional feature set as well.

The original design of homie was likely done with a very narrow use-case set in mind. And now we have a lot of mandatory topics that better sooner than latter should be changed to optional ones. The IP+MAC for example. As you said, not all devices might talk via IP networks and do have an IP and others are IPv6 (definitely multiple IPs per interface).

@davidgraeff
Copy link
Member Author

@ThomDietrich $implementation is fine for me, it just reads wired to have hardware related subtopics like $implementation/arch.

@timpur
Copy link
Contributor

timpur commented May 3, 2018

Just like to say something that is non related and irrelevant, but to everyone. The changes that we want to make should keep in mine of lowest resource clients, embedded devices. If we can support them, then homie will work for every client. So everything that we change has a cost, the more hierarchic means more ram needed for these devices. So we need to make sure that addle more hierarchical levels is worth this cost. The more topics has a cost, the more parsing and string format rules has a cost.

All i want to say is keep these things in mind, sometimes it is worth the cost, but sometimes it not.
(dont know where to put this ... sorry)

@Thalhammer
Copy link
Member

@timpur I totally agree with you, homie should always support small devices and we really need to make sure to keep the basic requirements small and easy to implement. But this works perfectly fine if we support optional featuresets. If you don't have enough resources, just don't implement them.

After I had time to read the actuall pull request I have some improvments:

  • Multiple MAC: If a device has multiple IP adresses it most likely will also have multiple MAC's and even if it does not, we should add it as an option or we might run into problems later on.
  • Optional but complete: If one of those topics in $system is implemented, all of them need to.

About versioning: If we use it as described it would be a bump of the major version number (e.g. 3.1.0 -> 4.0.0) since we break existing controllers if we remove required topics ($localip, $mac, $fw/*).

@davidgraeff
Copy link
Member Author

@Thalhammer It would be a break yes. At the same time it looks immature for the outsider if there are major version bumps like every three months. Version 3 got released just this year.

We wouldn't have this problem if there are multiple specs like Homie Core 1.0, Homie Device Statistics 1.0, Homie System Information 1.0 and so on. If one of those parts change (version bump), the rest can still be understood by a controller. The current situation is: /home/device/localip moves to somewhere else and because of that a controller is not able to do device discovery anymore?

@Thalhammer
Copy link
Member

@davidgraeff It will look even more immature if we claim to follow semver beginning with version 3.0 but break it right in the next release. Yes, moving localip might cause a controller to error out (Most, including mine, will not, but it never the less is a breaking change). I think a good way might be to just keep those properties where they are and add everything else which is not yet included as an optional subset. This would prevent us from having to increase the major version number and just bump minor (3.1->3.2).

However, as I already noted in an earlier answer, I would add some sort of sub specs.
Add a property $subsets with content like this system[1.0],somethingelse[2.0].
This might even be optional (not existing = empty) and could be done with just a minor version bump.
Describe each subset in a separate spec with its own versioning and keep the core convention as steady as possible. On next major bump move stuff like statistics into a separate subset and remove everything which is now obsolete from core. After this "cleanup" only add really important things to core (basically nothing). Each subset should have all its properties stored in a mqtt topic prefixed with the subset name, e.g.

/homie/device/system/* for subset "system"
/homie/device/stats/* for subset "stats"

This would allow continuing development of new features without touching core, help implement controllers (no need to update it so often, if a subset is not needed just ignore it), help to implement devices and keep resource usage low if needed (Just implement what fits into ram).

In addition, vendors could add there own property subset without risking to have naming problems in future versions.

@davidgraeff
Copy link
Member Author

davidgraeff commented May 5, 2018

I'm with you @Thalhammer.
I like to withdraw this PR and favour a split of the convention: #101

@davidgraeff davidgraeff closed this May 5, 2018
@ThomDietrich
Copy link
Collaborator

ThomDietrich commented May 5, 2018

The issue you were discussing about is actually what #96 and #51 are all about. I invite everyone to engage there.
The question wether or not something is "breaking" is handled a bit loosely by some it seems. Breaking down the convention into multiple parts/specs/files will "break" with one of the imho important ideas behind Homie -> Being simple and straight forward.

I'm answering here because it is too late to read #101 - I did not ignore it @davidgraeff 😃

@davidgraeff
Copy link
Member Author

Thomas. IMO what I have proposed as homie core is what the homie spec should have been. simple and straightforward. All those additional value topics like stats caused a lot of discussion.

@Thalhammer
Copy link
Member

@ThomDietrich Breaking Homie into multiple specs ensures exactly that. It keeps core simple and straightforward and removes the burden of having to implement a lot of stuff which might even be useless for some devices. #101 Would allow keeping the core steady and simple without preventing innovation.

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

Successfully merging this pull request may close these issues.

4 participants