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
Set Device type from query parameter in registration url #152
Set Device type from query parameter in registration url #152
Conversation
@@ -0,0 +1,45 @@ | |||
'use strict'; | |||
|
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.
License headers needed in all new files. You can copy paste from other existing file and add your self in a "Modified by:" entry at the bottom of the header.
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 in 376e415
(Same here)
@@ -7,6 +7,12 @@ config.server = { | |||
lifetimeCheckInterval: 1000, // Minimum interval between lifetime checks in ms |
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.
An entry in CHANGES_NEXT_RELEASE describing the change introduced by this PR should be added.
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 b934729
(It is a good practice from traceability reasons to include a mark like this specifying that the comment has been addressed and can be considered closed. I'm doing it for you this time :), but it would be great if you could do in the future for other PRs. Thanks!)
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 fully agree. It will be nice to write it down in the contributions guidelines. Thank you :)
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.
The "pull request protocol" is documented, but I'm afraid not in this repo but in this other: https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/contribution_guidelines.md#pull-request-protocol
A migration from there to the contributions guide in this repository is welcome! Basically it's a matter of copy-pasting and change repository name literals :)
@@ -7,6 +7,12 @@ config.server = { | |||
lifetimeCheckInterval: 1000, // Minimum interval between lifetime checks in ms | |||
udpWindow: 100, | |||
defaultType: 'Device', | |||
types: [ |
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.
Each time a new configuration item is added to config.js we need in addition:
- Describe it at https://github.com/telefonicaid/lwm2m-node-lib#-configuration
- Define a env var variable for it (LWM2M_XXXX) and add it to the table of in the above section
- Modify the code to process the env var as equivalent way of providing configuration
In this case I'm not sure about 2 and 3, as the value of this variable is an structured object, so it is not so simple. How the current code deals with similar case (i.e. config.formats)?
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 didn't add this variable and it is already described in the configuration section of the README.md file
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.
Ok, I understand you refer to this description in the README.md (https://github.com/telefonicaid/lwm2m-node-lib#-configuration)
The southbound interfaces can be configured in the
server.types
configuration parameter. This parameter is a list of objects composed of two attributes:
So you mean you have not added any new configuration variable in config.js but using and existing one. Is my undersrtanding correct?
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.
Yes
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.
Thanks for the clarification!
NTC
(NTC = Nothing To Change is another of the mark we use in pull request. In thi case, to tell that a given comments thread discussion is over)
Thanks for the contribution! Please find some feedback comments provided on 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.
LGTM
Thanks for the contribution! |
Hi,
following your code and the functionality of the services in lwm2m-iotagent, we found the device type functionality relies on the URI-Path prefix presented in the device registration process.
With a registration url
/Type/rd?ep=endpoint<=300&b=U
the device type will beType
if the type is defined in config.js.As far as I can read, this implementation does not comply with the standard specification of OMA LwM2M v1.0 [1][2].
Following the standard recommendation, this PR allows the server to extract the device type from the query string parameters, more specifically from the
type
query parameter.With this url
/rd?ep=endpoint<=300&b=U&type=Type
the device type will beType
if the type is defined in config.js.In this way, other lwm2m client implementations in which the URI-Path is not configurable can use the Device type functionality.
As you can see in the code, a new function returning the device type has been created in deviceRegistrationUtils. While device-registration-utils-test.js tests all the cases for this new function. The new functionality is optional and fully compatible with the previous code. The PR extends the number of third-party implementations that can use the lwm2m-node-lib server.
[1] http://www.openmobilealliance.org/release/LightweightM2M/V1_0-20170208-A/OMA-TS-LightweightM2M-V1_0-20170208-A.pdf
[2] OpenMobileAlliance/OMA_LwM2M_for_Developers#230