-
Notifications
You must be signed in to change notification settings - Fork 402
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
Enabled ability to force registration update #405
Enabled ability to force registration update #405
Conversation
Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>
@@ -214,4 +214,8 @@ public void removeObserver(LwM2mClientObserver observer) { | |||
public String getRegistrationId() { | |||
return engine.getRegistrationId(); | |||
} | |||
|
|||
public RegistrationEngine getRegistrationEngine() { | |||
return engine; |
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.
Do you really need to access to RegistrationEngine ? if not, I will go for an update() or forceUpdate() method.
Does it make sense to you ?
Probably off-topic: currently we support only one server but when we want support more I suppose we need to add a registrationID parameter to this method.
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.
You mean update() or forceUpdate() on LeshanClient directly? That would be OK too. Regarding the method naming, maybe the name triggerUpdateRegistration() is clearer. Just "update()" is not as clear, and "forceUpdate()" is maybe too harsh.
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 go for triggerRegistrationUpdate()
public void forceUpdate() { | ||
cancelUpdateTask(false); | ||
LOG.info("Forcing update of registration..."); | ||
updateFuture = schedExecutor.schedule(new UpdateRegistrationTask(), 100, TimeUnit.MILLISECONDS); |
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.
why using a delay ?
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 reasoning is that this is triggered by a server request, and a delay would decrease the risk of interfering with the response to that request. I'm no UDP expert, but a short delay seems less error prone. Maybe it should even be longer?
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.
It seems pretty binded to your specific use case. I will go for a scheduler.submit() and so you could add the delay you want on your side (this will be more flexible for 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.
Please clarify your last comment, I'm not sure what you mean.
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.
Replace schedExecutor.schedule() by schedExecutor.submit()
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
@@ -383,6 +383,12 @@ public void destroy(boolean deregister) { | |||
} | |||
} | |||
|
|||
public void forceUpdate() { |
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.
you should add if (started)
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.
Should probably be a synchronized method.
@@ -383,6 +383,12 @@ public void destroy(boolean deregister) { | |||
} | |||
} | |||
|
|||
public void forceUpdate() { | |||
cancelUpdateTask(false); |
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 think we need to interrupt the task. (false => true)
What do you think ?
Signed-off-by: Daniel Persson <daniel.p.persson@husqvarnagroup.com>
Integrated in master (commit cf933b6) |
For a LwM2m Server instance to be able to react on a Registration Update execute request, there needs to be a mechanism that can reschedule the update registration to occur immediately.
Signed-off-by: Daniel Persson daniel.p.persson@husqvarnagroup.com