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

First experimentation about Write Attributes support at cliend side. #1514

Closed
wants to merge 31 commits into from

Conversation

sbernard31
Copy link
Contributor

@sbernard31 sbernard31 commented Sep 19, 2023

This is experimental work to try to implement #534.

I tried to find solution to manage how to skip or delay notification. This a maybe a bit twisted but until now I can not find a simple way... 😬

We can not really do that at ObjectListener of LwM2mObjectEnabler because :

  • this will affect all listeners (not only listeners related to observation)
  • this will affect all observe relation but attribute are attached by server. (and even by request in LWM2M v1.2.x )

Note that :

  • I didn't test if this will fit with java-coap
  • I didn't think about Observe-Composite
  • the code is not complete but this is more a proof of concept which can be used as base for more development about that.

@dellzhui
Copy link

dellzhui commented Sep 20, 2023

Hi Simon,
You've started working on the client-side writeAttributes related to pmin vs pmax, which is a commendable piece of work, thank you!
I've combed through your upcoming patches and it looks like you've set up two separate asynchronous threads to handle the pmin and pmax parameters, can we merge them into a single thread to handle them because the meaning of the pmin and pmax parameters aren't completely independent, e.g., pmax must be bigger than pmin, and in java we don't use a coroutine like python etc., does a purely multithreaded design with support for concatenation affect performance?
My second question is, from the specification, both pmin and pmax are accurate to the leaf node, which means that there may be a conflict of settings on the server side, for example, for the object 6/0/1, the server can set /6, /6/0, /6/0/1, etc., respectively, because it may be a different person on the server side to set the attribute. Do we need to handle these pmin and pmax settings in the client's object tree in a unified way, so we can reduce the number of threads to be opened as much as possible?

@sbernard31
Copy link
Contributor Author

Thx for your feedback.

I'm not sure I really get your concerns in details.
But I understand that both concerns are mainly about using too many threads, right ?

The current code create several tasks but all tasks are schedule by a SingleThreadScheduledExecutor which means there is only 1 thread dedicated to execute all task related to Write Attribute by client by default.

@Warmek
Copy link
Contributor

Warmek commented Sep 28, 2023

Do attributes persist after de-registration and re-registration or do you have to send them again?
For both options, why?
Would it be possible to create a flag that could switch between these two behaviors?

@sbernard31
Copy link
Contributor Author

Do attributes persist after de-registration and re-registration or do you have to send them again?

I'm not sure sure. Are you asking what is specified in LWM2M ?

I find nothing about this is LWM2M specification, but you should have a look at it too and let me know if you find something.

I guess this is up to implementers to decide?

For both options, why?

You ask for pro/con about each solution, or something else ?

Without thinking too much about it :

  • persist :
    • pro : At first sight, you can save some bytes by sending less write attribute requests ?
    • con :
      • hard to find a good strategy about when data should be deleted at client side? (after a bootstrap ? only if related server is removed during a bootstrap)
      • if attributes are persisted server should remove it when it doesn't need it anymore, could be painful to maintained the desired state.
      • as this is not specified in LWM2M, how server could know if it should send write-attribute ? if it doesn't know device behavior it should send write-attribute anyway (to support all implementation).
      • even if server knows the behavior of specific devices, maybe client could lost its state and so a server should maybe do a discover before to send observe and so not sure we save a lot doing a discover instead of write-attribute.
  • no persist :
    • pro :
      • Housekeeping is simpler.
      • predictable behavior : new registration means no attributes. Server can consider device is clean at registration.
      • This is consistent with observe relation because this is specify that observe relation MUST NOT be persisted between 2 registration.

For a first implementation, I prefer "no persist".

Would it be possible to create a flag that could switch between these two behaviors?

I prefer to start with a minimal viable feature, code it properly, test it properly and make it works.
Then we will see if more complex behavior is needed.
This is not good for a project to add useless complexity. So we must be sure that a feature is really needed before to add it.

If this is really needed, I don't know if this will be directly added in the code base OR if this is just about adapt the API to let user implement their needs.

And also to be honest, I consider observe is already a very complex feature and we continue to add more complexity on it.
On my side, I strongly discourage it. I largely prefer the Send Request.

@Warmek
Copy link
Contributor

Warmek commented Oct 4, 2023

I didn't think about Observe-Composite
the code is not complete but this is more a proof of concept which can be used as base for more development about that.

What Attributes will be supported on Leshan Client? Why aren't they taken into account on Observe-Composite?

@sbernard31
Copy link
Contributor Author

What Attributes will be supported on Leshan Client?

I don't know. Potentially all attributes described in LWM2M v1.1.x.

Why aren't they taken into account on Observe-Composite?

I just mean that when I implemented this POC, I focus on "classic" observe not Observe-Composite use case.
Maybe this can be done later but the idea is to move step by step.

@mgdlkundera
Copy link
Contributor

@sbernard31 I have question about
NotificationManager.getAttributes(LwM2mServer server, ObserveRequest request)
You write in TODO to use objectTree to get attribute, can you explain what do you mean exactly by this?

@sbernard31
Copy link
Contributor Author

You write in TODO to use objectTree to get attribute, can you explain what do you mean exactly by this?

This PR is mainly a POC to ensure it's possible to modify default observe behavior, I don't know if this is the right way to go but when I write this code I had in mind that NotificationManager is just responsible to modify the default "observe behavior" based on "write attributes" set on the device.

So when a observe relation is established, NotificationManager need to get this "write attributes" currently set on the device.
I was thinking this will be accessible through objectTree but I didn't think too much how it could look like.

@mgdlkundera
Copy link
Contributor

@sbernard31 Where we should place data of Notification RW Storing When Disabled or Offline, in NotificationDataStore?

And another question is about data structure in NotificationDataStore how should look like data structure here and should be place there TimeStamped data?

@sbernard31
Copy link
Contributor Author

Where we should place data of Notification RW Storing When Disabled or Offline

I maybe missed something but "Storing When Disabled or Offline" is completely another feature ?

And another question is about data structure in NotificationDataStore how should look like data structure here

Data structure depends on needs.
If you don't want to support "multi server" and "composite observe" in first time, you can maybe go with something like :
a map<LwM2mPath, NotificationData>

should be place there TimeStamped data?

I didn't think too much about that but why would we need to timestamped data ? (which use case ?)

@mgdlkundera
Copy link
Contributor

It's related with SenML support on ObserveComposite, which Jarek will be doing, so we would like to know it, to think how to do it consistently for these issues

@sbernard31
Copy link
Contributor Author

It's related with SenML support on ObserveComposite, which Jarek will be doing, so we would like to know it, to think how to do it consistently for these issues

You're still talking about "TimeStamped data", right ?
I'm sorry, I may have forgotten something, but I don't get what you're referring to?
Maybe you are talking about : #1089

I didn't think too much of this. Generally I try to implement complex feature step by step.
So I define a simple minimal scope then I make it works.

I don't really advice to try to solve all problems at the same time.

@sbernard31
Copy link
Contributor Author

@mgdlkundera do you move forward on this ? I will maybe work on this again. So let me know so we can "synchronize our work"

@mgdlkundera
Copy link
Contributor

I'm working on it, I'll commit my changes today. Sorry I didn't answear but I was out of office in December.

@sbernard31
Copy link
Contributor Author

No problem, and no urgency, I was/am finally busy on others tasks 🙂

@mgdlkundera
Copy link
Contributor

@sbernard31
Copy link
Contributor Author

I will work on it again next week.
I looked at your commit and I will maybe get some part of it.

I don't know if you plan to work on it too ?
If you do, I'm not sure what is the right way to work on that together, maybe you could try to write some integration tests ? (of course for now some tests will not pass as all is not implemented 😅)

@mgdlkundera
Copy link
Contributor

So maybe I'll switch to another task. Let me know when you finish work on this and then I'll back to integration tests, is that okay?

@sbernard31
Copy link
Contributor Author

My idea about integrations tests is that they could probably be written without the feature implementation and so in parallel of my work.

But If you prefer to switch to another task this is possible too. 🙂

@sbernard31
Copy link
Contributor Author

I rebase that PR on master and begin to implement more dynamic code.
But it's hard to implement a feature which is not clearly specified ... (see : OpenMobileAlliance/OMA_LwM2M_for_Developers#478)

I plan to continue to work on this next week but maybe better to take a quick look to see if this go in the right direction.

@sbernard31
Copy link
Contributor Author

I push more work about write attribute support.
There is still missing point to implement to finalize a minimal viable feature.
But it should more or less work. (I guess there is still some bug)

I will write some tests to be able to test the code.

@mgdlkundera
Copy link
Contributor

I looked at your changes and from my point of view it goes in right direction.
Maybe I could work on this missing point or on something else?

@sbernard31
Copy link
Contributor Author

I looked at your changes and from my point of view it goes in right direction.

Great. Thx.

Maybe I could work on this missing point or on something else?

I summarize the missing point to do at : #534 (comment)

On my side, I plan to work on #1040, then integrate it in master once finished we will be able to rebase this branch on master and so implement the discover part.

I think that the point : "we also need to implement this for java-coap transport layer." should be done last. I think I should do as this could be not so easy. (not even sure this is doable 😓)

So if wanted you can try to finalize some missing point and add tests. (There is probably much more to tests)

If I understood you correctly this feature was needed (by Orange), so this could also be a good idea to start to test it in the context you plan to use it ?

@sbernard31
Copy link
Contributor Author

@mgdlkundera, @JaroslawLegierski, please let me know if you plan to test it ? 🙏

@mgdlkundera
Copy link
Contributor

I can test it in the next week

@mgdlkundera
Copy link
Contributor

I tested it and it seems to be okay.

@sbernard31
Copy link
Contributor Author

Thx for testing. 🙏

I didn't find time to integrate this in master sooner 😞 and next week I will not be available.
Sorry I will not be able to answer to your question until my comeback the 2nd April.

@sbernard31
Copy link
Contributor Author

Integrated in master commit (d8ef31a)

@sbernard31 sbernard31 closed this Apr 5, 2024
@sbernard31 sbernard31 deleted the write_attributes branch June 7, 2024 15:25
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.

4 participants