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

Encoder support for TimestampedLwM2mNodes #1246

Conversation

adamsero
Copy link
Contributor

No description provided.

@sbernard31
Copy link
Contributor

To contribute to Eclipse foundation project, you need to sign an Eclipse Contributor Agreement using the email used in your commit.

I just check for adam*****ski@****.com and it seems there is no valid ECA.

See How to contribute code§Legal stuff for more details.

Do not hesitate to ask if you have any issue.

@adamsero
Copy link
Contributor Author

The solution for #1228

@sbernard31
Copy link
Contributor

I just quickly check and there is formatting issue. (not a big deal this happens often with new contributor)

Are you using eclipse as IDE ?
If yes, you should look at https://github.com/eclipse/leshan/wiki/Code-&-design-guidelines#configure-your-ide
If you are using IntelliJ, previous contributors didn't find a good way to handle this : #1067
If you are using something else, I have no clue but let me know.

About this, I tried to add a formatting check at maven level but for now this is not a success : #1107

@adamsero
Copy link
Contributor Author

I used Eclipse to format, should be fine now.

Comment on lines 171 to 172
internalEncoder.records.get(0).setBaseTime(timestamp);
pack.addRecords(internalEncoder.records);
Copy link
Contributor

Choose a reason for hiding this comment

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

very cosmetic but maybe cleaner/easier to read to put internalEncoder result in a variable, then change the base time then use this variable to put in in pack ?

@sbernard31
Copy link
Contributor

I fixed some cosmetics issue :

  • a new formatting issue in DefaultLwM2mEncoder
  • javadoc in TimestampedMultiNodeEncoder (DefaultLwM2mDecoder => DefaultLwM2mEncoder)

I also fixed something which looks like a bug introduced with your last commit :

if (node != null) {
    node.accept(internalEncoder);

-    SenMLRecord record = internalEncoder.records.get(0);
-    record.setBaseTime(timestamp);
-    pack.addRecord(record);

+    List<SenMLRecord> records = internalEncoder.records;
+    if (!records.isEmpty()) {
+        records.get(0).setBaseTime(timestamp);
+        pack.addRecords(records);
+    }
}

I also adapted the test case to detect this issue ☝️ :

 @Test
public void senml_json_encode_timestamped_nodes() throws CodecException {
    TimestampedLwM2mNodes timestampedLwM2mNodes = TimestampedLwM2mNodes.builder()
            .put(500_000_001L, new LwM2mPath(0, 0, 0), LwM2mSingleResource.newStringResource(0, "TestString"))
            .put(500_000_002L, new LwM2mPath(0, 1),
                    new LwM2mObjectInstance(1, Arrays.asList(LwM2mSingleResource.newBooleanResource(1, true),
                            LwM2mSingleResource.newIntegerResource(2, 123))))
            .build();

    byte[] encoded = encoder.encodeTimestampedNodes(timestampedLwM2mNodes, ContentFormat.SENML_JSON, model);

    String expectedString = new StringBuilder()
            .append("[{\"bn\":\"/0/0/0\",\"bt\":500000001,\"vs\":\"TestString\"},") //
            .append("{\"bn\":\"/0/1/\",\"bt\":500000002,\"n\":\"1\",\"vb\":true},") //
            .append("{\"n\":\"2\",\"v\":123}]") //
            .toString();

    Assert.assertEquals(expectedString, new String(encoded));
}

@sbernard31
Copy link
Contributor

This is now integrated in master (commit a759c24)

@sbernard31 sbernard31 closed this Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants