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

Improvement #314

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improvement #314

wants to merge 1 commit into from

Conversation

slobglob
Copy link

@slobglob slobglob commented Feb 1, 2018

Added the report interval to the report in order to detect if the client has lost connection

Added the report interval to the report in order to detect if the client has lost connection
@tananaev
Copy link
Member

tananaev commented Feb 1, 2018

Can you please explain the use case.

@slobglob
Copy link
Author

slobglob commented Feb 1, 2018

@tananaev When the device sends a report, the device sends its report interval so the sever knows when to excpect the next report.
If the server doesn't get the report in that time (+some buffer) I know that the device went offline due to some reason (i.e. turned off or lost reception).

@tananaev
Copy link
Member

tananaev commented Feb 1, 2018

Are you planning to implement some modifications on the server side as well? What about iOS?

@slobglob
Copy link
Author

slobglob commented Feb 3, 2018

Maybe not right now, but possible to do that in the near future.
What do you think about that? Do you have any alternative to detect a GPS / Android loosing connection?

@tananaev
Copy link
Member

tananaev commented Feb 3, 2018

OK, we can merge when everything is ready. Also, the right way of implementing this would be to add a new parameter to the Position model. One more thing I suggest is renaming parameter to something shorter (e.g. "interval").

@slobglob
Copy link
Author

slobglob commented Feb 4, 2018

I'm not sure if that's correct, the interval isn't really a part of the Position model attributes (logically).
Just because we want to send it to the sever doesn't mean it has to be a part of the model.
The attribute name can definitely be changed.
And OK, we'll wait until all platforms are aligned.

@tananaev
Copy link
Member

tananaev commented Feb 4, 2018

I think it makes sense to send original interval value at the time when location was captured.

@slobglob
Copy link
Author

slobglob commented Feb 4, 2018

If you think this is the best approach we can do that, but I'm not sure that's the right way logically.
I'll keep you posted on the other platforms so we can merge everything at the same time.

What exactly do I need to change in the server?

@tananaev
Copy link
Member

tananaev commented Feb 4, 2018

We can either keep server as it is, but ideally we want to use interval for "unknown" status timeout.

@slobglob
Copy link
Author

slobglob commented Feb 5, 2018

Ok, I'll keep you updated. Thanks.

@garyvdm
Copy link
Contributor

garyvdm commented Feb 12, 2018

I like this idea, and would find it useful.

@oliv3
Copy link
Contributor

oliv3 commented Sep 15, 2018

@slobglob @garyvdm #348 implements this and more, if you want to try, feedback welcome.

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