Add Timestamped Nodes support to Send Operation at Client side

[sbernard31]

Following #1192 we decide to add Timestamped nodes to Send Operation.

The Server side part was done by #1218.

This issue aims to track / discuss about client side support.

(Here some idea about a DataCollector ? => #1192 (comment))

[adamsero]

Hi, my name is Adam and I replaced @Michal-Wadowski as a contributor from Orange Polska. I've been studying the code and I made some first steps and committed them on opl/timestamp_nodes_support_for_client.

I created a basic encoder for type TimestampedLwM2mNodes, with optional support for non-timestamped multiple nodes (in that case it uses the old encoder). So far I only tested it in a crude way, but I'm currently working on proper integration tests.

As for now, it is possible to send nodes with timestamps, but it's not possible to retrieve them from enablers - I'd like to discuss how to approach this topic after you approve of the encoder/tests.

[sbernard31]

(I will look at this tomorrow)

[sbernard31]

Hi Adam, welcome on board 😉

I looked quickly at the code and that sounds to go in the right direction.

So far I only tested it in a crude way, but I'm currently working on proper integration tests.

I'm not sure you will be able to start by integration test. You will probably need to define the client API before you write integration tests ?

But you can start to add encoders unit tests.

As for now, it is possible to send nodes with timestamps, but it's not possible to retrieve them from enablers - I'd like to discuss how to approach this topic after you approve of the encoder/tests.

About client side API, I'm not sure it makes sense that enablers returns timestamped value.
I rather have the idea of a kind of DataCollector (#1192 (comment))

The idea is to have an object which is responsible to collect data from enablers, timestamped the data, store it, then we use this stored data to send SendRequest later.

e.g. I configure my DataCollector to collect /3/0/9 (battery level) each 10 minutes. this will get data like with LeshanClient.collectData(), and timestamp it with current timestamp then store it in memory. Then e.g. each 2 hours I can send the collected data.

This is not crystal clear to me how this should looks like exactly but I think this could be a good way to explore. What do you think ?

Anyway, If you really need timestamp from enablers, please let me know the use case 🙂

[sbernard31]

(By the way I will be unavailable from the end of day to Monday 18th april, back on Tuesday)

[adamsero]

Alright, I will work on unit tests for encoders then - you're right about that.

About client side API, I'm not sure it makes sense that enablers returns timestamped value.
I rather have the idea of a kind of DataCollector

I agree with that, it's better not to modify the existing structure of enablers, but rather add new functionalities on top of the existing ones. However writing a DataCollector is going to be a lot of work.

[sbernard31]

However writing a DataCollector is going to be a lot of work.

I'm not so sure. 🤔

I guess we should define an interface with a very simple default implementation in a first time.
Then user will be able to provide their own implementation or we will be able to enhance it.
Maybe in a first time we don't even need to provide an automatic periodically way to collect. (just a manual way is enough?)

Do not hesitate to ask for help if needed.

[adamsero]

I will think about how to implement this idea while you're away and hopefully have something ready by the time you're back 🤞.

As for now, I added some unit tests for encoding in SenML-JSON and SenML-CBOR (f6ca130)

[sbernard31]

@adamsero any news about this ? 🙂

[adamsero]

I think I should have something to show later today when it comes to Data Collector. In the meantime, do you possibly know if the next release is going to contain changes from #1218? And if so, when will it come out? It's fairly important for Orange to have these changes in a release sometime soon.

[adamsero]

@sbernard31 OK, in 00baa08 I added DataCollector as an interface to client core and TemperatureDataCollector as a sample implementation to client demo. You can start and stop periodic read on a resource via an enabler and retrieve collected timestamped data at any time.

Right now there's no real way to test this in practice, so I had to override send interactive command so that it passes the data collector to sendData in LeshanClient just to be able to send it to the server. Having said that, I though about proper control over data collectors:

• Add a separate interactive command that starts/stops periodic read on a specified resource (it could also create the collector, although that can be done statically in code)
• These collectors would be mapped by the resource path, so Map<LwM2mPath, DataCollector>
• Add an option to send command that causes data to be read from the collector instead of the enabler. Collector would be retrieved by the path provided by the user

Let me know what you think about my code and the ideas 🙂

[sbernard31]

In the meantime, do you possibly know if the next release is going to contain changes from #1218?

Yes, next release will integrate this.

And if so, when will it come out?

The plan I had in mind was : #1222.
=> Waiting for OSCORE and timestamped nodes for send to release the 2.0.0-M7.

It's fairly important for Orange to have these changes in a release sometime soon.

What could be a good release date for you ?

(Note that I will be often unavailable during May)

[sbernard31]

About DataCollector, I looked quickly at your code. You get the idea but I think we need to take time to specify this more precisely. I will try to take to time on this.

Waiting and as first step you can create a PR with encoder modification + unit test about it (without data collector)? This should be an easy PR and so probably a first good step to enter in Leshan project ?

Add a separate interactive command that starts/stops periodic read on a specified resource (it could also create the collector, although that can be done statically in code)

Yes I also have this kind of idea about adding some command about this in leshan-client-demo.

These collectors would be mapped by the resource path, so Map<LwM2mPath, DataCollector>

For now I don't know if we should have several DataCollector or only 1.

Add an option to send command that causes data to be read from the collector instead of the enabler. Collector would be retrieved by the path provided by the user

Same here I don't know if the collector is responsible to send or not the data.

I really need to think more about all of this. 🤔

[adamsero]

I created a PR #1246

[adamsero]

What could be a good release date for you ?

It would be convenient for us if you could release a version with timestamped Send before OSCORE, especially since you mentioned you might be less active in May.

[sbernard31]

It would be convenient for us if you could release a version with timestamped Send before OSCORE

You mean the server part only ? or also the client part which is currently in development ?

Do you have an approximate date in mind ? end of April ? mid May ? end of May ?

[sbernard31]

@adamsero let me know that ☝️ as soon as possible (especially if you need it for end of April, because there is not so much time remaining 😅 )

Did you try to integrate the current master branch to be sure it behave like you expect ?

[sbernard31]

The next release will be mainly for you and I'm asking myself if we should add the last californium release 3.5.0 to this release ? Currently we depends on cf 3.4.0.

[adamsero]

Sorry for the delay, but I had to contact team members and we established that the latest date would be around mid-May. It will be enough if only Server-side changes are in the release, but I don't know if we can make it in time to also include Client-side. We could have PR #1246 in the release at the very least (if you even want to include it).

[sbernard31]

OK so I think we can integrate #1246 and #1247.
For mid-May, I bet we will not be able to include the DataCollector in time.

I will be unavailable from April 29th to May 8th (included), So either I try to release before the end of the week or when I will be back the 9th.

(Note I will be unavailable again from May 13th to 22th and also 26/27th, then I will be avaialble again as usual)

[adamsero]

If you could make it by the end of the week, it would be wonderful 🙏. The new californium version is not a priority, especially if it would slow you down.

[sbernard31]

OK so let's focus on #1246 and #1247, then I try to make a release.

[sbernard31]

Tomorrow is my last day of work for April, so I need to integrate #1246 and #1247 in the morning to be able to release in time.
So if you want to do some modification you have until tomorrow morning. 😁
If you want to stop me 😅 , let me know.
If you haven't time to polish regarding to my comments on those PRs, not a big deal I will do the little cleaning myself 😉.

[sbernard31]

(spoiler alert, this #1224 could prevent me to release tomorrow)

[adamsero]

I made changes to both branches, should be good now.

[sbernard31]

(spoiler alert, this #1224 could prevent me to release tomorrow)

Lucky me, today the github quota was not reached , so release is ongoing. 😌

[adamsero]

As my previous example with sender identified by name was about adding sender dynamically (pool1, pool2 ...).

Oh, so in that example you could create pools at the same time as you collected data, got it. I was thinking more like:

1. User creates a sender with a command, giving the newly created sender a unique name.
2. When collecting/sending, user specifies to/from which sender and they have to enter a name of a sender that already exists. I guess they could also enter a new name along with an option like -create or something.

But we'll think about that another time, for now I'll implement what I described previously.

[adamsero]

I pushed the changes in 0647aad

[sbernard31]

I looked at this quickly and I don't get why DataSender interface has those methods ? 🤔

void collectData(List<LwM2mPath> paths);

void sendCollectedData(ServerIdentity server, ContentFormat format, long timeoutInMs, boolean noFlush);

[adamsero]

Those methods are being called from DataSenderManager, and since it uses Map<String, DataSender> to hold the senders, I figured DataSender interface had to contain the definitions. I guess we could theoretically only have them in ManualDataSender and then downcast every instance of that class so we can call these methods - but the periodic data sender will probably also have the same methods in common, so I just put them in the interface.

[sbernard31]

OK but I'm not sure I also get why DataSenderManager also exposes some collectDataManually and sendDataManually method delegating it to a ManualDataSender.

For me there is no API defined for a Sender except setter(s) to give it useful tool (collect/send) to implements its logic.
A user just creates its own sender, then put it in the client (eventually with a name)
Then it keep a reference to it (or retrieve it by name) when it need to call specific API which allows to control it.

But maybe I missed something ?

[adamsero]

For me there is no API defined for a Sender except setter(s) to give it useful tool (collect/send) to implements its logic.
A user just creates its own sender, then put it in the client (eventually with a name)
Then it keep a reference to it (or retrieve it by name) when it need to call specific API which allows to control it.

I understand your logic, but I proposed a different approach (in my previous comment):

Every operation (collect/send) will go through the manager, which then will tell specified sender to collect data and hold it in its memory or retrieve collected data and send it.

and you agreed on this, so I just did what I said I would do.

[sbernard31]

Oops, sorry I didn't understand it like this 😬

I'm not sure I get how it will works with something else than manual sender ?

[adamsero]

Well, IMO there can really only be two types of senders: manual or automatic - users can create their own types but they will just be extensions of the aforementioned ones. Since automatic senders (including periodic ones) don't require user interaction by definition, you only really need to provide access to the manual ones through the manager.

Of course, users don't have to use that option if they don't want to, but since the manager already aggregates senders, I thought it would be a good idea to have some utility methods like collectDataManually or sendDataManually - they just help with managing multiple manual senders.

You have a good point about limiting DataSender's API, so in 234d23b I just left the setDataSenderManager method and removed the rest. I also added methods to retrieve senders from the manager by name, in case users implement their own senders differently and just want to access the sender object to then call their own methods.

[sbernard31]

Since automatic senders (including periodic ones) don't require user interaction by definition, you only really need to provide access to the manual ones through the manager.

I'm not sure I agree 🤔
E.g. if you want to change configuration dynamically you will need to access to automatic sender API. I can imagine a use case where you create a LWM2M object which control DataSender and so API can not be abstract.

Or even leshan-client-demo do configure collect and send period.

Of course, users don't have to use that option if they don't want to, but since the manager already aggregates senders, I thought it would be a good idea to have some utility methods like collectDataManually or sendDataManually - they just help with managing multiple manual senders.

In my opinion this add more code for not so much benefits because :

• We will not really be able to do a whole abstract of manual sender. (We can imagine there is more than just collect/send maybe listCollected or persistcollected)
• I feel that maybe abstracting those 2 methods are just too leshan-client-demo oriented.

So I would prefer to keep it simple and so remove the methods in manger, but If you really want to added, let's go for it.

[adamsero]

OK, I can remove the collectDataManually and sendDataManually methods, but I believe I should keep
DataSender getDataSender(String senderName)
and
<T extends DataSender> T getDataSender(String senderName, Class<T> senderSubType)
since that will make it available for users to use the manager as a container for senders as well. Of course they don't have to hold them there, but it would be a nice feature IMO.

I'm not sure where to put the logic from the removed methods though, should it be directly in the run() in command's class?

[sbernard31]

OK, I can remove the collectDataManually and sendDataManually methods, but I believe I should keep
DataSender getDataSender(String senderName)
and
<T extends DataSender> T getDataSender(String senderName, Class<T> senderSubType)

That sounds good.

I'm not sure where to put the logic from the removed methods though, should it be directly in the run() in command's class?

Yep I think it makes sense too.

[adamsero]

I think it might be ready to integrate into master, unless you want it more polished?

[sbernard31]

Maybe I missed something but do you create a PR ? (or push --force on an existing one ?)

[adamsero]

No worries, I just wanted to wait for you to confirm it's finally ready. Unfortunately, there's going to be a merge conflict if I open a PR from opl/manual_data_sender due to the latest commit 30bad68 on master. The forked repo maintainer from our company is on vacation, so I can't update our forked master to resolve the conflict. Any ideas what to do with that? He'll be back on Monday if anything.

[sbernard31]

No worries, I just wanted to wait for you to confirm it's finally ready.

I didn't look at this yet, anyway PR is right place to discus about details in the code.

Unfortunately, there's going to be a merge conflict if I open a PR from opl/manual_data_sender due to the latest commit 30bad68 on master. The forked repo maintainer from our company is on vacation, so I can't update our forked master to resolve the conflict.

I'm not sure I get you ? you mean the master branch of przem83/leshan repo is 1 commit behind Leshan's master, correct ?

But what is the problem exactly you don't know how to fetch this commit ? or you don't have the right to push on master in przem83/leshan ?

[adamsero]

or you don't have the right to push on master in przem83/leshan ?

That's it

[sbernard31]

Ok so you can fetch the commit 30bad68 and rebase your work on it.
When you will create the PR you will create it againts master from leshan repository (not przem83 one), so you don't care "your" master (in przem83) is 1 commit behind.

I hope I'm clear.

[adamsero]

By 'fetch it' do you mean open a PR from eclipse/master to przem83/manual_data_sender? If I do that, it shows me there will be conflicts:

If not, then I don't know what you mean by that. Sorry about my lack of knowledge, but I haven't worked with a public github repo and forks yet.

[sbernard31]

By 'fetch it', I mean git fetch this commit from leshan repository.

I guess if you have conflict you didn't do it. (That's why I asked if "you don't know how to fetch this commit " ?)
I wanted to know if you wanted some help about it ?

In your local git repository you can have several remote repositories attached.
Each repository have a name.
Often beginner works with only 1 remote repository called origin.
But you can add the official Leshan repository as remote too. (you can name it leshan)
Then you will be able to fetch master from this repository.
Once it's done, juste rebase your work on commit 30bad68, then push force your branch again.

# I suppose your current remote repo is named origin
# -------------------------------------------------------
# add leshan repo
git remote add leshan [email protected]:eclipse/leshan.git
# fetch leshan master branch
git fetch leshan master
# checkout your local branch if not already done 
git checkout manual_data_sender
# rebase it on leshan/master  (commit 30bad6824) 
git rebase leshan/master
# you will probably need to solve conflict
# once it's done, push force the branch 
# (if you are not so sure of you, you  can create a local branch of previous origin/manual_data_sender branch)
git push origin  manual_data_sender --force

If I didn't missed anything, you should be able to create a PR on leshan repo without conflicts.

[sbernard31]

(another solution just push the PR with the conflicts and I will resolve it when I will integrate it in master but I guess that maybe it could be useful to learn that kind of git stuff ☝️ )

[adamsero]

I created #1268 but it didn't pass the code check - it looks like the compiler doesn't understand the Consumer interface in a forEach loop - since it's a Java 1.8 Interface, my question is can we actually use 1.8 or do I have to stick to 1.7?

[sbernard31]

I'm currently trying to add more automatic check (see #1265)
Some comment should have been automatically added but it seems that there is some right issue with Leshan repository that I didn't face with my own repository (sbernard31/leshan)
I will try to investigate this.

About your question, this issue(#924) should give you some answer.

I see that your PR contains merge commit. For Leshan we don't use it, rebase only.
I prepare a code check about this : #1267
(By the no need to recreate new PR when this is about the same changes)

If you have more question relative to your PR, I think it's better to ask at #1268.

[sbernard31]

@adamsero, #1274 is now integrated in master, I think we can close this issue.

Do you want to work on Periodic Sender ?
From my point of view this is not a vital feature to have but if you want to work on this it's OK to me 🙂

In all case, I think we can close this issue and when needed create a new issue to talk about it ?

[adamsero]

Yeah, I think we can finally close this one now. I'm not sure about Periodic Sender's priority right now considering #1222, but I might open it as a new issue at some point in the future. At the moment I have some side tasks but after I'm done with them we can think more about the issue.