Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LOC: request device configuration and publish metrics/info #3037
LOC: request device configuration and publish metrics/info #3037
Changes from all commits
5027b3e
45f6828
8611b62
525c857
e227d2e
01f06a7
1bdc2f3
2ba30e4
374ed65
576993c
a629a6f
9ddbd5c
5f40052
74a5671
393cc1a
1c4011e
91f617a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@milan-zededa don't we want to publish periodically to AllDest here? (and, if so, rename the tcloudTicker variable)?
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.
With the current code in the PR we only publish location to the LOC when we publish all info which is when the controller epoch changes.
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 should publish location with the
appTicker
below. That on is firing at a higher frequency (timer.location.app.interval
= 20secs) thancloudTicker
(timer.location.cloud.interval
= 1 hour). The idea is that LPS (and probably also LOC) are much closer networking wise and under much less stress than zedcloud, so they should be able to show more frequent location updates.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.
But see below - I might have missed a new use of triggerPublishAllInfo(). Makes sense to add comments about that new usage here resulting in publishing the location and check that the comment for triggerPublishAllInfo isn't stating something misleading abouts its usage.
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.
Not sure I understand comments right. The default behavior is unchanged. If not - this is a bug.
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.
Since you are now also refactoring a bit, would you rename this to
publishLocationToLPS
? Also any other occurrence of LocalServer can be renamed to LPS to avoid confusion.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.
@milan-zededa Sure? 'LocalServer' is used in many places all over the code. I can, but won't this only increase confusion if I change only the function name?
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 wouldn't mind if you renamed all of it :)
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.
Not in this PR. Here I offer only one function rename! :)