-
Notifications
You must be signed in to change notification settings - Fork 3
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
writeProperty message #38
Open
benfrancis
wants to merge
1
commit into
w3c:master
Choose a base branch
from
benfrancis:writeProperty
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me these are two different things. Write-only means that the value of the property cannot be read. The ability to confirm that a write was completed is a separate concern.
If the underlying hardware offers no such confirmation then a 'completed' response would mean that the write was applied. I would still like to see a confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a property that can't be read and a write that can't be confirmed could be argued to be subtly different things. However, unconfirmable writes do exist.
For example, in the X10 communication protocol some devices do not acknowledge the receipt of commands. You can send a command to switch a device on, but there's no way to tell whether that command was received.
My concern would be that if a Web Thing sends a confirmation message when a write may not actually have been completed or received, it may be misleading.
As far as I know a Thing Description has no way to distinguish a confirmable
writeproperty
operation from a non-confirmable one (other than by using thewriteOnly
member of a PropertyAffordance).Therefore my suggestion would be that a Web Thing that can not confirm whether a write operation has succeeded (for whatever reason) should use the
writeOnly
member as a hint, and a Consumer should not treat a lack of a response as an error condition in that case. An error message could be sent in response if the write is definitely known to have failed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I would really like to differentiate between
propertyReading
and an acknowledgment that the write request or invokeAction request was processed using the best available method. Even if a write operation cannot be confirmed, the Thing provider should still be able to acknowledge that the request was received and acted upon to the best of its ability.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a need for a message type to acknowledge a
propertyWrite
message without providing a property reading, then yes it does seem like that would need to be separate message type frompropertyReading
. Adding a special "propertyWriteAcknowledgement" message type feels wrong though, so if this really is important then maybe this is further evidence for the need to split out operation name and message type.I've filed #42 to explore this further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through the proposal and I also find this point very sensitive. My main concern his how a Consumer should behave when writing a property. If the response "acknowledge" (either we decide to use
propertyReading
or dedicatedack
) MIGHT arrive, then the best behaviour is to never wait for something, but rather request again a property read (if not write-only). If we don't have the ability to specify if a property write is acknowledgable by the TD, I think the default should be "there is no acknowledge".I would explore the possibility to introduce the term
acknowledge: true
as a custom protocol binding keyword.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My vote goes to Ben's proposal in #42:
This addresses the concern I raised as there is always a response. The response can carry a value if the output is available, no value if the property is writeonly, or an error if it fails. This is deterministic and simple. The Affordance already states whether the property is writeonly so there is no guesswork involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't catch up with the discussion. Then I definitely support the direction above!