-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add Test Object (3441) to LWM2M Registry. #639
Add Test Object (3441) to LWM2M Registry. #639
Conversation
1d02642
to
abaa243
Compare
- XML schema does not match LWM2MVersion value. - Description2 is not well placed.
abaa243
to
5b3de65
Compare
@bocajim, @mlasch, @qleisan, @tuve, @rettichschnidi could you review this ? 🙏 |
I like this but, how long is a string? Pun intended but the question is serious, when randomizing values would also the string length be randomized? Couldn't optional arguments be added to the randomize resource to control the length of the string and the number of multiple instances. This in itself is a valid test case. |
@tuve thx for feedback About reset/clear/randomize arguments I have the idea that we could add as argument the resource ids which should be reset/cleared/randomized (no args means all resources) but I finally don't add it because I'm not sure this is really useful 🤔 About your concern about string size and multi value size, I'm not sure I get it, could you be more precise ? 🙏 Currently the random function is more for "visual"/"playing" test, I mean using leshan-server-demo you can observe the object instance than click on randomize and see values changes but I agree this is maybe not so useful too write real/deterministic tests. In this case maybe is better to use Write Request with precise value ? |
Testing the support for arguments, but it may also be useful to test that resources of a certain size are supported. This can of course be achieved deterministic by writing. |
OK so what do you think about the idea above ? |
sounds like a good idea
Den fre 10 sep. 2021 kl 19:10 skrev Simon ***@***.***>:
… Testing the support for arguments
OK so what do you think about the idea above ?
exec on resource clear with arguments 10, 20 will clear only resource 10
and 20 ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#639 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFJVCY253VVTCRMCJPS6MDUBI3X7ANCNFSM5DZOHFPA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Replace the file name from 3441-1.0.xml to 3441-1_0.xml.
(@jpradocueva thx for fixing file naming 🙏) @tuve About testing support for arguments of executable resource. So another idea is to create 2 other resources :
WDYT ? |
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.
Suggesting small changes but overall LGTM.
3441.xml
Outdated
<LWM2M xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:noNamespaceSchemaLocation="http://openmobilealliance.org/tech/profiles/LWM2M.xsd"> | ||
<Object ObjectType="MODefinition"> | ||
<Name>Test Object</Name> |
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.
More descriptive name would be good (perhaps something like "LwM2M Specification Test Object"?)
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 changed the name by LwM2M Specification Test Object (commit 972ab9f)
xsi:noNamespaceSchemaLocation="http://openmobilealliance.org/tech/profiles/LWM2M.xsd"> | ||
<Object ObjectType="MODefinition"> | ||
<Name>Test Object</Name> | ||
<Description1><![CDATA[This object contains resources for each datatype allowing easy interoperability testing.]]></Description1> |
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.
More details or reference to a spec with more details about how this is supposed to be used could be good here too.
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 think that this could be a next step. Asking the person who did a lot of work already to do more work appears to be unfair to me.
3441.xml
Outdated
<RangeEnumeration /> | ||
<Units /> | ||
<Description>Set random value to all resources. For | ||
multiple value, the number of resource instances is |
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.
meaning of "multiple value" is not clear here
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 think it should say "For multi-instance resources, the number of resource instances is also randomized."
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 changed it (commit be3c599)
I also add some advice about limiting size of randomized value. (See #639 (comment))
<Type>Opaque</Type> | ||
<RangeEnumeration /> | ||
<Units /> | ||
<Description>Initial value must be "0x0123456789ABCDEF". |
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.
How is the value encoded? In binary I suppose (not as ASCII characters)?
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'm not sure to get the point.
This is an Opaque resource, so not limited to ASCII chars.
About encoding it depends of ContentFormat used by the request.
(I feel I missed something 🤔)
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 meant how the value "0x0123456789ABCDEF"
is encoded to the resource. I assume you mean this is a sequence of 16 octets that have values (shown in hex) "0x0123456789ABCDEF". Alternative interpretation would be sequence of ASCII characters.
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.
Correct, 0x0123456789ABCDEF is the byte array value in Hexadecimal.
Do you feel the description is not clear enough ?
Should I replace by :
Initial value must be 0123456789ABCDEF (Hexadecimal notation)
OR
Initial value must be 0x0123456789ABCDEF (Hexadecimal notation)
OR
any other better idea ?
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.
Both are better than the original but if you want to be even more clear I would suggest something like:
Initial value is sequence of 16 octets with value (shown in hex) "0x0123456789ABCDEF".
I wonder if there is some precedent of such description in another object we could replicate here. But you probably know best what is the most clear description for developers.
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 changed it (commit daabe13)
3441.xml
Outdated
<Type>String</Type> | ||
<RangeEnumeration /> | ||
<Units /> | ||
<Description>Initial value must be 1 instance with id 0 |
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.
<Description>Initial value must be 1 instance with id 0 | |
<Description>Initial value must be 1 instance with ID 0 |
(and same with other instances of "id" too)
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.
fixed by commit 8c7ee24
Does it has to be multi instance resource? In a real life use case it kind of doesn't make sense to have an exec just overwrite another resource when you just as easily could do a write, but since this is a test object, it would be easier to use in tests and implement. One exec resource with argument that writes the argument to a read only resource? |
Thanks for doing this work! We discussed this object at the last conference call and I have solicited reviews from the group. This is certainly a useful object for future interoperability events. |
A couple of notes from me: This object seems to be somewhat similar in intent to the private I'm not suggesting to use our object instead - our object is rather Anjay-specific (it includes resources that only make sense in the context of Anjay internals), however I believe that some aspects of our version may be worth including in the official object:
These are just loose suggestions - you may or may not want to include before publishing this object, or they might be included in a subsequent version of it. |
I just checked the test Object we are using internally. Let's just say that "great minds think alike". As @kFYatek wrote, adding the 1.1 and 1.2 data types would be nice. I also suggest having two additional resources: one read-only and one write-only. Regards, |
The idea is to reserve : - 100 to 999 for RW single resources. - 1000 to 1999 for RW mult-instance resources. - 0 to 100 for exec or "special" test resources.
About Including all data types from LwM2M TS 1.1 and 1.2 @dnav, @kFYatek, as explained in this PR description :
I plan to put another version of the Object which targets LWM2M 1.1. |
About testing duplicate,
@kFYatek , I'm not so sure for now so I didn't add it. But maybe if more feedback in favor of this, we can add it. |
About adding Read and Write only resource,
@dnav, I'm not sure to understand if you want to :
I guess this is rather 1) Maybe I totally missed your point, so please do not hesitate to elaborate on this. |
I updated Leshan PR to be compliant with last modification see eclipse-leshan/leshan#1093. |
My suggestion was item 1. The goal was to test that allowed operations are respected. But this can be done with already existing Objects. So forget about it. |
I don't know if this should have been merged. At least there are this #639 (comment) which is not addressed. |
🙏
Now the object is merged, I'm not sure to understand the process to clarify the "0x0123456789ABCDEF" description ?
You mean :
My idea initially was rather 2) but I vaguely understand that we rather go with 1). Do I get correctly ? |
Simon,
Yes, the test-object branch is still available. Please make a pull request to revise.
The group (OMA) agreed to allocate 3441 for lwm2m 1.0 xsd test, 3442 for 1.1 and 3443 for 1.2
Does this help?
Cheers
Matt
From: Simon ***@***.***>
Sent: Tuesday, September 28, 2021 12:09 PM
To: OpenMobileAlliance/lwm2m-registry ***@***.***>
Cc: Gillmore, Matthew ***@***.***>; State change ***@***.***>
Subject: [EXTERNAL] Re: [OpenMobileAlliance/lwm2m-registry] Add Test Object (3441) to LWM2M Registry. (#639)
In support of your project we wanted to move forward with this submission.
🙏
If you wish to clarify how "0x0123456789ABCDEF" is encoded, we can always revise the object.
Now the object is merged, I'm not sure to understand the process to clarify the "0x0123456789ABCDEF" description ?
Should I create a new PR againts the test-object branch ?
OMA met today and we agreed to support a lwm2m 1.0 object and in the future separate objects for 1.1, 1.2, etc
You mean :
1. a new Object with a different ID and name ?
2. or a new version for this Object (so with same ID and name) ?
My idea initially was rather 2) but I vaguely understand that we rather go with 1). Do I get correctly ?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https:/github.com/OpenMobileAlliance/lwm2m-registry/pull/639*issuecomment-929371883__;Iw!!F7jv3iA!gMicOK1bLhuo6bk2awkwINh3aAk1Cu3HP5_7Kt1PS228DmUWbPtbndcOKuExb6RFgpCr$>, or unsubscribe<https://urldefense.com/v3/__https:/github.com/notifications/unsubscribe-auth/AJSSA5T2QVX3FKSJD6QBVXLUEHSDHANCNFSM5DZOHFPA__;!!F7jv3iA!gMicOK1bLhuo6bk2awkwINh3aAk1Cu3HP5_7Kt1PS228DmUWbPtbndcOKuExb4yP15Vl$>.
Triage notifications on the go with GitHub Mobile for iOS<https://urldefense.com/v3/__https:/apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!F7jv3iA!gMicOK1bLhuo6bk2awkwINh3aAk1Cu3HP5_7Kt1PS228DmUWbPtbndcOKuExb5M3XQy5$> or Android<https://urldefense.com/v3/__https:/play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!F7jv3iA!gMicOK1bLhuo6bk2awkwINh3aAk1Cu3HP5_7Kt1PS228DmUWbPtbndcOKuExbyYsOOQ6$>.
|
I understand now.
As 2. was chosen, should we change the name accordingly ? I feel that |
Thinking a bit more about those names seem too long to me so maybe :
|
I created a new PR #642 for final polish of this object for LWM2M v1.0. I think the last missing point is the name question.
Once we agree with that I will provide a PR for LWM2M v1.1 Test Object. Thx all for your valuable feedbacks 🙏 ! |
Add new Test Object to LWM2M Registry.
The idea is to have an object which could help to do some interoperability test.
(Maybe it could even be used during test fest ?)
Any other idea/resource addition is welcomed too.
This Leshan PR (eclipse-leshan/leshan#1093) allows to test the
Test Object
.This PR followed discussion at #623.
(Corresponding issue #636)