Skip to content
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

LwM2mNodeSenMLDecoder for SenML-JSON with unit tests. #681

Conversation

cavenaghi9
Copy link
Contributor

LwM2mNodeSenMLDecoder for SenML-JSON with unit tests, inspired by LwM2MNodeJsonDecoder.

Signed-off-by: Boya Zhang [email protected]

// Extract baseName
LwM2mPath baseName = extractAndValidateBaseName(pack, path);
if (baseName == null)
baseName = path; // if no base name, use request path as base name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About "request path as basename, if basename is absent", I read quickly the specification §7.4.4 JSON, I didn't find anything about that.

It seems this part has changed with the new JSON format. I guess this is to respect more the SenML rfc. (If this is the case, I approve this change)

Anyway, I maybe missed it so if you find anything let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read-Composite is introduced in LwM2M 1.1. Table: 7.4.4.-6 JSON payload in the server request to read manufacturer name, battery level and registration lifetime shows an example of Read-Composite request like below:

[{"n":"/3/0/0"},
{"n":"/3/0/9"},
{"n":"/1/0/1"}]

In response to the above Read-Composite request the client will return a JSON payload as below:
[{"n":"/3/0/0", "vs":"Open Mobile Alliance"},
{"n" :"/3/0/9", "v":95},
{"n":"/1/0/1", "v":86400}]

In this case, the base name is absent, because of resources belong to different object instances, and object instances belong to different object. Do you think SenML JSON/CBOR encoder/decoder shall consider Read-Composite/Write-Composite/Observe-Composite now? Will appreciate if you share you opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect our current API is not really compatible with this.
So I would go to let this aside for now : one step at a time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(when I say "let this aside", I mean support of Read-Composite/Write-Composite/Observe-Composite)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(when I say "let this aside", I mean support of Read-Composite/Write-Composite/Observe-Composite)

Understood! Is there any plan or on-going contributing activity related with Composite operation now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no plan about that for now.
About on-going contribution, I just saw this #563 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After extract base name from record array, I am trying to update value of name field of each of record to full path. Therefore, rest of logic can ignore base name anymore. Do you have any comments on that?

Copy link
Contributor

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 understand. I think we should remove extractAndValidateBaseName(pack, path) because we could face cases where there are several basenames ?

look at this https://tools.ietf.org/html/rfc8428#section-5.1.6

   [
     {"bn":"2001:db8::2/","bt":1.320078429e+09,
      "n":"temperature","u":"Cel","v":25.2},
     {"n":"humidity","u":"%RH","v":30},
     {"bn":"2001:db8::1/","n":"temperature","u":"Cel","v":12.3},
     {"n":"humidity","u":"%RH","v":67}
   ]

if we want a more LWM2M one we could imagine something like this :
Reading object /2 (ACL) containing 2 instances

   [
     {"bn":"/2/0/","n":"0","v":3},
     {"n":"1","v":1},
     {"n":"3","v":123}
     {"bn":"/2/1/","n":"0","v":4},
     {"n":"1","v":1},
     {"n":"3","v":124}
   ]

(we could add unit tests like those)

So basename would be now handle in groupRecordsByInstanceId, maybe like this :

    private static Map<Integer, Collection<SenMLRecord>> groupRecordsByInstanceId(Collection<SenMLRecord> records)
            throws CodecException {
        Map<Integer, Collection<SenMLRecord>> result = new HashMap<>();

        String basename = "";
        for (SenMLRecord record : records) {
            // update basename if present
            if (record.getBaseName() != null)
                basename = record.getBaseName();

            // Build resource path
            String path = record.getName() == null ? basename : basename + record.getName();
            LwM2mPath nodePath = new LwM2mPath(path);

            // Validate path
            if (!nodePath.isResourceInstance() && !nodePath.isResource()) {
                throw new CodecException(
                        "Invalid path [%s] for resource, it should be a resource or a resource instance path",
                        nodePath);
            }

            // Get SenML records for this instance
            Collection<SenMLRecord> recordForInstance = result.get(nodePath.getObjectInstanceId());
            if (recordForInstance == null) {
                recordForInstance = new ArrayList<>();
                result.put(nodePath.getObjectInstanceId(), recordForInstance);
            }

            // Add it to the list
            recordForInstance.add(record);
        }

        return result;
    }

private static Map<Integer, Collection<SenMLRecord>> groupRecordsByInstanceId(Collection<SenMLRecord> records,
LwM2mPath baseName) throws CodecException {
Map<Integer, Collection<SenMLRecord>> result = new HashMap<>();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following comment above, I feel that we don't need baseName parameter anymore.
And we should just use baseName from SenMLRecord.
Look at this part of the rfc to implement it correctly : https://tools.ietf.org/html/rfc8428#section-5.1.6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above comments, I am trying to update value of name field of each of record to full path, then base name can be ignore, any comments for this change?

// Create an entry for an empty instance if possible
if (result.isEmpty() && baseName.getObjectInstanceId() != null) {
result.put(baseName.getObjectInstanceId(), new ArrayList<SenMLRecord>());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no more baseName parameter, we will probably remove this part of the code.
And so we should handle the case where result if empy in calling method.

if (baseName == null)
baseName = path; // if no base name, use request path as base name

// Group JSON entry by instance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a details, but several time in comments you are using JSON or JSON entry, I feel this would be more appropriate to use SenML or SenML record.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that, I tried to replace or remove irrelevant comments, but still missed something.

Signed-off-by: Boya Zhang <[email protected]>
@sbernard31
Copy link
Contributor

(@cavenaghi9, just to let you know I will not be available next 2,5 weeks, so don't worry if I don't reply)

@cavenaghi9
Copy link
Contributor Author

(@cavenaghi9, just to let you know I will not be available next 2,5 weeks, so don't worry if I don't reply)

2.5 weeks or 2~5 weeks? having a nice vacation.

@sbernard31
Copy link
Contributor

2.5 weeks or 2~5 weeks? having a nice vacation.

That was 2.5 week 😁

@cavenaghi9
Copy link
Contributor Author

2.5 weeks or 2~5 weeks? having a nice vacation.

That was 2.5 week 😁

Are you available now, not sure if your vacation is finished? I have to go on contribution.

@sbernard31
Copy link
Contributor

I have been back since May 9, but next week I will be unavailable too.

But anyway do not hesitate to contribute at any time. I just share my availability to avoid you to worry if I don't answer in time.

- update value of name field of each of record to full path, using name field directly when creating LwM2mPath object.
- remove used codes, and update description.

Signed-off-by: Boya Zhang <[email protected]>
This reverts commit d279478.

Signed-off-by: Boya Zhang <[email protected]>
- update value of name field of each of record to full path, using name field directly when creating LwM2mPath object.
- remove used codes, and update description.

Signed-off-by: Boya Zhang <[email protected]>
@cavenaghi9 cavenaghi9 force-pushed the lwm2mNode-senML-json-decoder branch from 9ed4b34 to 85eeb21 Compare June 26, 2019 01:54
@sbernard31
Copy link
Contributor

I'm a bit confuse with the current state of this branch.
Could you please squash all the commit and rebase this PR on branch 2.0.x ?

@cavenaghi9
Copy link
Contributor Author

squash

I'm a bit confuse with the current state of this branch.
Could you please squash all the commit and rebase this PR on branch 2.0.x ?

I forgot to sign-off, then I changed commit history.
I changed unit test in #682 , therefore I merge changes of 2.0.x to current branch.

any comments or question please let me know.

@sbernard31
Copy link
Contributor

I will not be available the next 2 weeks but I look at this as soon as I'm back.

Could you please squash all your commit and rebase it over branch 2.0.x ?
(do no hesitate to tell me if this is an issue for you)

@sbernard31 sbernard31 added this to the 2.0.0 milestone Aug 14, 2019
@sbernard31
Copy link
Contributor

@cavenaghi9 do you plan to continue to work on this PR ? (it's just to know, no pressure :))

@sbernard31 sbernard31 added the orphan PR with no more activity label Nov 22, 2019
@sbernard31 sbernard31 closed this Jul 16, 2020
@sbernard31 sbernard31 reopened this Jul 16, 2020
@sbernard31 sbernard31 changed the base branch from 2.0.x to deprecated_2.0.x July 16, 2020 16:58
@sbernard31
Copy link
Contributor

I finished and integrated this PR in #911.

@cavenaghi9 thx again for all your contribution. 🙏

We didin't get any news from you for long time ago now. It seems you are no more active on github. I hope all is fine for you.

@sbernard31 sbernard31 closed this Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
orphan PR with no more activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants