-
Notifications
You must be signed in to change notification settings - Fork 81
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
fix(coap-server): add missing cov:observe subprotocol #1214
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1214 +/- ##
==========================================
+ Coverage 77.57% 77.74% +0.16%
==========================================
Files 83 84 +1
Lines 17332 17443 +111
Branches 1751 1761 +10
==========================================
+ Hits 13446 13561 +115
+ Misses 3850 3846 -4
Partials 36 36 ☔ View full report in Codecov by Sentry. |
402878d
to
04064c5
Compare
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 have some questions below before approving the PR.
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.
Good to go now.
Thank you! I incorporated the latest change into 55e197c, squashing the commits. Hope that was fine. |
Trying to interact with a test thing using CoAP, I noticed that the forms provided in the Thing Descriptions do not include the
subprotocol
valuecov:observe
for observable properties and events. This PR tries to fix the issue by adding logic to determine if asubprotocol
should be added to a form.The implementation got a bit more complicated than initially expected because I noticed that you probably should not simply add the subprotocol to all property forms, but only those containing the op values
observeproperty
and/orunobserveproperty
. Furthermore, I got the feeling that you should not usecov:observe
withreadproperty
orwriteproperty
(which I think is currently not clearly stated in the CoAP Binding Template).Therefore, during the TD generation process, I split the op values for read/write operations and observe/unobserve operations, and generated separate forms for the two kinds of operations. There might be some more discussion and clarification in the protocol binding document needed to decide whether
cov:observe
should or should not be used with forms containingreadproperty
and/orwriteproperty
. In general, the approach in this PR should work regardless of the eventual decision, though.Besides the actual implementation, I performed some minor refactoring of the process of assigning the forms to affordances and added a test to cover all changes made.