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
modem_cellular: add RSRP &RSRQ to U-blox SARA-R5 modem #67025
modem_cellular: add RSRP &RSRQ to U-blox SARA-R5 modem #67025
Conversation
I'd like to request some help from @jeffwelder-ellenbytech or @bjarki-trackunit regarding the failed pipeline. It fails because my added functions aren't used by the Quectel BG95 modem instance, which warning causes the sample "test" to fail. It feels messy in the long run to ifdef all such parsing functions depending on which modem use them. I think modem_cellular.c is quite hard to navigate as it is due to the instantiation of the different modems, and now the API is added in the bottom where normally the device instantiation is. This feels like a larger discussion that should be taken elsewhere, because it feels like this structure of one giant .c file won't scale well when APIs are implemented, and more modems and AT commands are added. Do you agree? I'm in the process of migrating from an out-of-tree mod of |
Hi, I know, I didn't look close enough to the actual API implementation that was added in modem_cellular.c and I'm looking into fixing it :) In short, API implementations will go into the struct device API pointers, and startup/periodic scripts will only be used if absolutely necessary, CESQ for example will to into its own script run synchronously when the API is called |
See #67066 |
@bjarki-trackunit The PR looks fine, I guess, but it doesn't address the problem in this PR (if that was the purpose). The thing is that Thanks! |
You should be able to rebase and add CESQ to the driver now that #67066 has been merged :) |
f4318e9
to
e49206d
Compare
@bjarki-trackunit please take a look! |
e49206d
to
09236e0
Compare
Looks good! I will test that it handles CESQ not being supported nicely as well (Quectel only supports CSQ) |
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.
Works as expected on the BG95, gets RSSI just fine, fails properly when trying to get RSRQ/RSRP (since it does not support AT+CESQ)
[00:00:22.941,131] <inf> modem_cellular: event script success
Signal RSSI -51
[00:00:23.980,895] <wrn> modem_chat: get_signal_cesq_chat_script: aborted
[00:00:23.980,957] <inf> modem_cellular: event script failed
Failed to get RSRP
[00:00:25.020,965] <wrn> modem_chat: get_signal_cesq_chat_script: aborted
[00:00:25.021,057] <inf> modem_cellular: event script failed
Failed to get RSRQ
[00:00:25.063,171] <inf> modem_cellular: event script success
Signal RSSI -51
09236e0
to
5301aa0
Compare
This commit implements parsing of the CESQ extended signal quality AT command, extracting RSRP and RSRQ which is relevant for LTE connections. It's used in the U-blox SARA-R5 modem instance. Furthermore, the IMEI, IMSI is extracted in the same modem. Signed-off-by: Emil Lindqvist <emil@lindq.gr>
5301aa0
to
d35be40
Compare
This commit implements parsing of the CESQ extended signal quality AT command, extracting RSSI, RSRP and RSRQ, the latter two being relevant for LTE connections. It's used in the U-blox SARA-R5 modem instance. Furthermore, the IMEI and IMSI is extracted in the same modem.
A couple of comments:
modem_cellular_chat_on_csq
feels wrong. Suggestions welcomed (maybe return -ENODATA if invalid?)