-
Notifications
You must be signed in to change notification settings - Fork 8
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
app: enable GNSS based on device shadow config #165
Conversation
edba646
to
dcdb0c8
Compare
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.
Looks fine, added a few comments to consider.
app/prj.conf
Outdated
CONFIG_LOCATION_METHOD_GNSS=n | ||
CONFIG_LOCATION_METHOD_GNSS=y | ||
CONFIG_LOCATION_SERVICE_NRF_CLOUD_GNSS_POS_SEND=y | ||
CONFIG_LOCATION_SERVICE_HERE=n |
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.
Looks like this one already defaults n?
CONFIG_LOCATION_REQUEST_DEFAULT_GNSS_TIMEOUT=200000 | ||
CONFIG_LOCATION_REQUEST_DEFAULT_TIMEOUT=300000 | ||
CONFIG_LTE_NETWORK_MODE_LTE_M_NBIOT_GPS=y | ||
# CONFIG_LOCATION_LOG_LEVEL_DBG=y |
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.
Unused, should we just remove 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.
I've been turning it on and off multiple times, and I figured it would be handy to have there easily available during development. We can remove it before launch.
app/src/modules/location/location.c
Outdated
/* GNSS has to be enabled after the modem is initialized and enabled */ | ||
err = lte_lc_func_mode_set(LTE_LC_FUNC_MODE_ACTIVATE_GNSS); | ||
if (err) { | ||
LOG_ERR("Unable to init GNSS: %d", err); |
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.
This might be an irrecoverable error. I suggest treating it as such.
app/src/modules/location/location.c
Outdated
|
||
if (status == NETWORK_CONNECTED) { | ||
/* GNSS has to be enabled after the modem is initialized and enabled */ | ||
err = lte_lc_func_mode_set(LTE_LC_FUNC_MODE_ACTIVATE_GNSS); |
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.
This function only needs to be called once. Having a guard here saves few cycles in the long run. No big deal though.
There is also a few sonarcloud issues that might be worth looking into |
c428417
to
cd67822
Compare
This commit enables GNSS in kconfig and dynamically requests gnss from the location library depending on the config from the device shadow. Co-Authored-By: Simen S. Røstad <[email protected]> Signed-off-by: Gregers Gram Rygg <[email protected]>
cd67822
to
5cc032b
Compare
Quality Gate passedIssues Measures |
} | ||
|
||
if (status == NETWORK_CONNECTED) { | ||
/* GNSS has to be enabled after the modem is initialized and enabled */ |
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.
For OOB this is fine, but for trackers with buffering capabilities we probably want to use GNSS before we have connectivity (i.e if we are out of coverage)
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.
Sure. This was just the easiest way to enable gnss when the modem was ready. The modem_init (or similarly named) callback was triggered before the modem was ready to enable gnss.
This commit enables GNSS in kconfig and dynamically requests gnss from the location library depending on the config from the device shadow.