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

[LR11x0] GNSS support #1275

Merged
merged 16 commits into from
Oct 18, 2024
Merged

[LR11x0] GNSS support #1275

merged 16 commits into from
Oct 18, 2024

Conversation

jgromes
Copy link
Owner

@jgromes jgromes commented Oct 16, 2024

Based on the work that has been done in #1216 - full credit to @tve for that!

I've reworked the GNSS API to make it more easily usable. I've also split the single example sketch into three different examples (almanac update from satellites, autonomous position and satellite information dump).

Couple more small cleanup steps are still missing (keywords, some fixes etc.), but this is mostly ready.

@jgromes jgromes self-assigned this Oct 16, 2024
@jgromes jgromes marked this pull request as draft October 16, 2024 20:15
src/modules/LR11x0/LR11x0.h Outdated Show resolved Hide resolved
@jgromes jgromes marked this pull request as ready for review October 18, 2024 14:26
@jgromes jgromes merged commit 00699ce into master Oct 18, 2024
30 checks passed
@jgromes jgromes deleted the lr11x0-gnss branch October 18, 2024 14:50
@jgromes
Copy link
Owner Author

jgromes commented Oct 18, 2024

With the last few fixes, I think this concludes the GNSS suport for LR1110/20. To be honest I'm mildly disappointed by the performance, as the error I was seeing was always in the range of 10-15 km, even with 7+ satellites. It remains to be seen whether the performance with LoRaCloud will be better.

@tve
Copy link

tve commented Oct 18, 2024

To be honest I'm mildly disappointed by the performance, as the error I was seeing was always in the range of 10-15 km, even with 7+ satellites. It remains to be seen whether the performance with LoRaCloud will be better.

This is exactly what I wrote in the discussion. Using the cloud service you get the usual ~10 meters of accuracy.

Why this is so: the location produced by the chip itself is based on almanac data, which is coarse satellite trajectory info. The chip does not download ephemeris data, which is fine-grained satellite trajectory info and is necessary to produce a more accurate position.

@lyusupov
Copy link
Contributor

lyusupov commented Nov 20, 2024

@jgromes

Jan, what was the reason for reverting this previously applied PR ?

image

Tip

Why would not to store a "cache" of ConfigLfClock() argument in a private area of class LR11x0 and disable DIO11 output only when external 32768 Hz XTAL is activated by RADIOLIB_LR11X0_LF_CLK_XOSC selector ?

@jgromes
Copy link
Owner Author

jgromes commented Nov 20, 2024

@lyusupov good question - probably just an oversight on my part. Looks like that change happened in 74a8d9a, however, there's many other changes in that commit as well, so I'm not 100% certain it was intentional. I'll test whether this is really necessary and if not, I will return it to the state that was merged in #1275.

@lyusupov
Copy link
Contributor

I suppose that the idea was to test beginGNSS() with connected 32K XTAL (DIO10 - DIO11)

image

However,

  1. are there so many LR11XX devices or modules that have this 32K crystal connected ?
  2. does beginGNSS() apply to LR1121 that does not have GNSS scanning feature ?

I have nothing against this new GNSS + XTAL feature but it could make sense to enable it only when the 32K crystal is really connected and involved in operation. But default behavior should be to use DIO11 as an IRQ signal.

@tve
Copy link

tve commented Nov 20, 2024

Because, as stated in AN1200-74:
image

@jgromes
Copy link
Owner Author

jgromes commented Nov 20, 2024

@lyusupov looks like @tve is correct, DIO11 is required for GNSS scanning. Setting any sort of configuration for DIO11 in this case breaks advanced GNSS scanning. The clean way to resolve this is what you sugest, checking whether GNSS scanning is enabled and if so, reserve DIO11 for that purpose.

jgromes added a commit that referenced this pull request Nov 21, 2024
@jgromes
Copy link
Owner Author

jgromes commented Nov 21, 2024

@lyusupov FYI, changed in ef16e43

@lyusupov
Copy link
Contributor

@lyusupov FYI, changed in ef16e43

Great job, thanks!

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.

3 participants