Stateful attribute for properties

[davidgraeff]

Stateful attribute for properties

Signed-off: David Graeff [email protected]

[lorenwest]

Is there a reason this property wasn't called Retained?

Stateful has many meanings, one of which may be a retained behavior, yet it looks like you're using Stateful to mean retained. Retained is an MQTT verb, and I recommend keeping terminology consistent.

In your example, the state of the doorbell indeed does change - even though that state may not be remembered (retained in the MQTT broker). Stateful doesn't seem to represent the true meaning of this new attribute.

[davidgraeff]

@lorenwest True point. This PR is based on the discussion in #70 though. I didn't want to bring yet another word into the game. I'm not with you regarding the doorbell example. A (doorbell) button (in contrast to a switch) can be seen as stateless. I'm talking about the event "PRESSED". I guess you are thinking about button "states" DOWN and UP.

[lorenwest]

MQTT uses the word Retained for this, so if the motivation is to encourage non-MQTT usage of Homie, introducing a different term is beneficial. I've never considered the applicability of Homie outside of MQTT.

[davidgraeff]

I have adapted the PR to use "retained".

[bodiroga]

Shouldn't the last sentence be "Publishing to a non-retained property topic MUST always happen with the MQTT 'retain' flag 'false'."? IMHO, the 'off' word could be confusing.

[marvinroger]

"with the MQTT 'retain' flag off" means without the retain flag, not with the flag set to 'off', so I'd say it's fine.

[bodiroga]

Also, shouldn't we update the Homie version number? As far as I know, we are now in the 3.0.0 version, so perhaps the new version should be 3.1.

[davidgraeff]

There is a rather long discussion on how to increase version numbers. I thing the status quo is not to increase the version number for non breaking changes.

[bodiroga]

LGTM! 👍

[ThomDietrich]

Hey @davidgraeff 👋 the PR looks good to me. From a logical point of view "stateful/stateless" seemed easier to grasp but retained is an established and clearly understood word in MQTT terminology - hence 👍

In the comparison of the four combinations you could stress the difference between "state" and "event" even more.

Regarding version numbers: #46 every PR (commit to master) is supposed to increment the version number! It depends on the level of change which version number part has to be increased. We discussed this here #96 but did not yet reach a decision.

The PR introduces a new attribute that changes the nature of the property slightly.
On client side this feature is a pure addon feature -> no breaking change.
On controller side this feature does not change the basic interaction with a property as a non-retained message can be processed just as any other -> no breaking change.
Do you agree? Under this argument I believe this PR calls for a PATCH increment.

[davidgraeff]

@ThomDietrich The current document contains the 3.x.x version tag, which stays I guess. So the version number increase would not be done in this PR but instead by tagging the commit?

[ThomDietrich]

Correct. At the point of changing it to "3.x.x" I thought that changing this line of the README every time a PR is proposed will create unneeded chaos. (Still do)
marvinroger suggested once to run a webpage to serve the Convention. When this becomes a reality, the "3.x.x" can dynamically be replaced (see VERSION placeholder)

[bodiroga]

Any remark @timpur? Is the PR ready to be merged? 👍

[davidgraeff]

Guys, if a PR simple like this takes 2 weeks there is something seriously going wrong with the maintainership.

[ThomDietrich]

I agree. After my feedback last week I expected @marvinroger or @timpur to do the next step. Not everyone is always online. Let's give them till Saturday.

@davidgraeff if you'd be interested and @marvinroger agrees it might be an option to give you repository contributor permissions.

[davidgraeff]

A structured way on how this convention should be evolved would help. Like:

• Is a majority of active maintainers required OR a is peer review sufficient?
• How long is the voting process for a feature. (Close issues older than that amount of time as "Invalid" or "Rejected")
• How do you determine if a maintainer is active?

I propose the following:

• A new discussion/feature can be raised by a GitHub Issue or a Pull-Request. A Pull Request is preferred, as it allows to talk about a specific idea more easily.
• The discussion period for a new feature lasts for a maximum of 3 weeks. Everyone involved can apply for a longer time frame if required. 3 weeks no progress means "Rejected" automatically.
• Active maintainers list themselves in a MAINTAINER.md file. Everyone in that file is required to response within 3 weeks to an Issue. A maintainer can remove/add himself at any time to the mentioned file if workload/holiday will prevent him participating in a foreseeable future.
• All active maintainers MUST agree on a new feature for it to be added.

[ThomDietrich]

My opinion on this:

• I'm against the "close inactive tickets after x weeks". An issue is either resolved or closed by discussion or it should be left hanging to ripen, e.g. to be picked back up at a later point. This will be helpful when a discussion regarding a certain topic comes back up (see "won't fix" label).
• I think we should follow the four-eyes principle. A PR is approved by one maintainer and merged by another. The merging one can be the creator of the PR. Before the merge everyone can raise critique and for important changes other users and/or maintainers should be mentioned to request their opinion. An exception is the trivial case (spelling error, ...) in which a single maintainer is sufficient.
• I agree we should add a MAINTAINER file

[davidgraeff]

@ThomDietrich This will over time fill the Issues/Pull-Requests with open Issues/PRs. As soon as there are so many that they not fit on one page, people will start to forget about them and no discussion will ever happen on those.
If the original poster or someone else is really convinced on having his feature in the convention, it is his responsibility to keep the discussion alive and summarize from time to time.

If you are worried about people only looking at open Issues/PRs before posting another feature request -> Those exact people are the ones that will not scroll through all pages of open issues to find a match. Everybody should use the search function first for open/closed issues before raising another one.

[marvinroger]

Hi guys,

I created an issue for the maintainership problem: #109

[marvinroger]

LGTM for me 👍
Before going ahead and merging this, I do like these versioning rules: #96 (comment)

As this is a property addition, it should be minor-bumped instead, don't you think?

[davidgraeff]

@marvinroger I would say this is a patch version update only. Because an optional attribute with a default equal to the status quo is added.

[jamesmyatt]

It's not clear to me what the value in publishing the "retained" message is. Can you clarify?

[davidgraeff]

@jamesmyatt It is a "boolean" type, so it accepts "true" and "false" with a default implicit value of "true"

[jamesmyatt]

@davidgraeff, sorry if the question wasn't clear. I meant why is it published? What value does that add? In what situation would a subscriber use that information?

I think that the clarification that about combinations of settable/non-settable and retained/non-retained is very valuable, though.

[davidgraeff]

Everything in Homie is per default a retained topic. It is impossible to publish an event-like value (push button pressed). With the ability to set the retained flag to off (former name was "stateful" / "stateless"), you can publish events in accordance to the Homie specification.

[bodiroga]

Hi @jamesmyatt!

This comment from @ThomDietrich describes perfectly the four different combinations with settable/non-settables and retained/non-retained options: #70 (comment). As david has told you, the $retained flag allows controllers to know if the property is stateful or stateless 😉

[marvinroger]

I would say this is a patch version update only. Because an optional attribute with a default equal to the status quo is added.

Makes sense. Thanks!

[lorenwest]

@davidgraeff - Small input: Any change to the API should be a minor version bump. Making this change as a patch would put it on the same level as fixing typos, and it's much more than that. As an implementer of the API, I'd like to say I'm API version Major.Minor compatible. I don't need to update the implementation for typos, but I would want to update it for minors.

[jamesmyatt]

I agree with the clarifications.

I also agree that it should be a minor version bump (i.e. v3.1) as it's a new feature.

I just don't know when a subscriber would use the "retained" topic, i.e. what automated logic would react to it? I understand how values were previously retained by default, but MQTT things should be designed such that retained vs non-retained is invisible to the subscriber: if a subscriber needs a value that they didn't observe and never subscribed to to work correctly, then it should be retained, otherwise it can be retained or QoS 1+. You don't need a "retained" topic to allow values to be published non-retained.

[davidgraeff]

@lorenwest, @jamesmyatt: This is not an API change, a revision increment is sufficient. I'll explain:

For a current controller implementation nothing changes: The implicit default of "retained" is true, as it was before. A controller that does claim to support "3.0.1" can additionally handle non-retained topics different if it wants too.

For a current device implementation nothing changes as well.
Additionally: If you want to describe your published topics as event type topics (retained=off), you would tell controllers that you have implemented "3.0.1".

[lorenwest]

@davidgraeff - According to the semver spec:

• MAJOR version when you make incompatible API changes,
• MINOR version when you add functionality in a backwards-compatible manner, and
• PATCH version when you make backwards-compatible bug fixes.

You've made the argument that this API addition is backwards compatible, placing it in the MINOR position. If this were a bug-fix (typo for an API), it would be suited for the PATCH position.

Sorry to be a stickler on this minor issue, but as the maintainer of a package that has thousands of dependents and millions of downloads a month, if I got this wrong in node-config I'd be slaughtered...

[davidgraeff]

@lorenwest I agree, that according to semver you might understand this as a minor update. Except if you change your perspective a little. If you make something explicitly settable (like the retained flag), that was implicit before, you could argue, that this is a "bug fix".

Why is it important to not increment version numbers fast: We want implementers to adopt this specification / convention. Not everybody knows about semver, and a minor version looks like a huge change for a lot of people (mostly because big projects handle minor versions different like the linux kernel, libre office, and so on).

If we convey the impression that this specification is evolving too fast and people can't keep up with their homie projects, we shoot ourself in the foot.
We already have the problem that a lot of implementations are stuck at version 2.
And because this addition is really little, why not opt for the revision version increase.

Because this is a community project, I'd say we have a small vote here.
Please use the little reaction emoji function for this post.

• Thumbs up: You follow my argumentation above and the revision version increment stays.
• Thumbs down: You follow the semver rules, and this change will become a minor version increment instead.

I'll have a look at the results on the 02. Nov.

[lorenwest]

Can I both like your post, and vote thumbs down at the same time? :)

You make compelling arguments from the perspective of someone that hasn't experienced the value of semver, which as you point out, may be the majority of our audience. Soliciting votes is a great way to gather data on this dynamic.

To be fair, fudging on MINOR/PATCH isn't a terrible thing, and you outline some fair advantages. Getting MAJOR/MINOR wrong, however, can have lasting negative effects.

[jamesmyatt]

Anyone who cares about version numbers knows about and uses semver. It's a settled problem; semver has won.

OK, I'm exaggerating, but not much.

[fermuch]

We want implementers to adopt this specification / convention. Not everybody knows about semver

If it is specified that we use semver for versioning, then it is not something new (it is quite common), nor is it difficult to read about if you didn't know about it.

I'm implementing all my things under the idea that semver is respected. If you introduce a breaking change but didn't feel like it was good for branding to increment the major version, then I might not be able to apply a patch for that specific version (and I shouldn't, we're using semver after all).

I'm OK if we switch from semver to something else, but please make it really clear how changes are introduced. After all, $homie is used inside the protocol (by machines), it is NOT a branding only feature.

[davidgraeff]

I clarify my point of view: This is NOT a breaking change at all. It does NOT influence how controllers work with the topic data and a device IS NOT required to send or interpret this new attribute :) It is a "Bux fix" from the perspective of semver.

[lorenwest]

I can understand the argument to use semver or not using semver, but if we say we adhere to semver we lose our ability to make our own rules, and I must emphatically object to masquerading a non-breaking API addition as a bugfix.

This is absolutely a change to the API, as an implementer I understand this well. This is absolutely not a bug fix or patch, because what bug is it fixing?

To be clear, the only discussion here is about either adopting semver or not. I believe arguments have been made to adopt our own versioning, with our own page along with reasons behind this argument.

We can't say we're semver compatible without being semver compatible. It's our community responsibility.

[davidgraeff]

I'd like to bring this document as reference: https://tools.ietf.org/id/draft-claise-semver-00.html
It is about adopting semver for IETF Specifications. This is a quote from the document:

In IETF terms, this versioning scheme provides functionality
analogous to several parts of the traditional IETF process.

* MAJOR is analogous to an “obsoletes” relation between RFCs
* MINOR is analogous to an “updates” relation between RFCs
* PATCH is analogous to use of the RFC errata process

Personally I would say the change is equivalent to an RFC errate.
Something becomes explicit which was implicit before.

This is semver for documents/specifications, not for software. There is not much expertise outside on how to handle those situations. That is the reason for the poll and this very interesting conversation.

[lorenwest]

How can an API addition (the ability to express retention along with a new $retained verb) be considered errata? Before this change there was no way of expressing $retained. After this change it has two states. Clients listening to the device should be able to examine API version to see if it supports this expression.

If we're semver compatible, there's exactly one place to express non-breaking features. If not semver, we can put it anywhere.

One of the major values of semver is it takes versioning out of the hands of the marketing department. Considerations of how this looks is a marketing concern. I'm not saying it's an invalid concern, but it doesn't belong in the conversation if we're semver compatible.

[marvinroger]

I agree with @lorenwest. Even though this is an addition making something that was implicit explicit, it is still an addition. Plus, being able to express stateless properties is a feature. So I'm for the minor bump, too.

[ThomDietrich]

Vivid conversation, I like it!

I want to quote my comment from earlier:

The PR introduces a new attribute that changes the nature of the property slightly.
On client side this feature is a pure addon feature -> no breaking change.
On controller side this feature does not change the basic interaction with a property as a non-retained message can be processed just as any other -> no breaking change.

I believe we all have to consider that the convention is not easily comparable to an API - for which semver is primarily designed and described.

For every change to the conversation we need to evaluate the impact of the change on the expected behavior of the receiving partner.

Let me give this example: Imagine a 3.0.1 client announcing/sending a non-retained property to a 3.0.0 controller. The controller does not know the added feature but is able to handle the property flawlessly, except for it's statelessness.
The client is not hindered in it's ability to communicate with the controller and the controller can discover a client and exchange data and events with the client to the extend of the underlying v3.0.0 convention.

Following the illustrated example this PR very much qualify as a PATCH version bump.
@lorenwest and others: I'm very interested in your counter arguments. Which part of my example would you not agree with?

[timpur]

@davidgraeff @bodiroga @marvinroger @ThomDietrich @lorenwest , I apologies for my absence, ive been overwhelmed with work lately, but starting to get back in the balance of work / social time. I will be actively catching up on what i missed and will try to respond asap. Sorry for any annoyance of my MIA.