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

drivers: cellular: add rssi / modem information #65685

Merged
merged 3 commits into from Dec 12, 2023

Conversation

ldenefle
Copy link
Contributor

@ldenefle ldenefle commented Nov 23, 2023

Hello, this PR is an effort to bring a method to pull things like RSSI or modem / SIM information from an app.
It leverages the new periodic script and the begining of the cellular API implemented by @bjarki-trackunit .
The PR right now is not in a mergeable state but I'd be keen for maintainers to have a look at it and start a conversation around the fundamentals of it:

  • does it fit in the architecture ?
  • are there shortcomings I'm not aware of ?
  • is it too early for a PR like this ?

Comment on lines 71 to 79
uint8_t imei[CELLULAR_MODEM_INFO_IMEI_SIZE];
/* GSM model as a string */
uint8_t model[CELLULAR_MODEM_INFO_MODEL_SIZE];
/* Manufacturer name as a string */
uint8_t manufacturer[CELLULAR_MODEM_INFO_MANUFACTURER_SIZE];
/* International Mobile Subscriber Identity */
uint8_t imsi[CELLULAR_MODEM_INFO_IMSI_SIZE];
/* Integrated Circuit Card Identification Number (SIM) */
uint8_t iccid[CELLULAR_MODEM_INFO_ICCID_SIZE];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a large list of const strings, I will suggest struct of const char *

struct cellular_modem_info {
        const char *imei;
        const char *model;
        ...

where the members point to char buffers initialized by the modem driver. Then when the modem has initialized the char buffers, we only need to copy pointers to the internal buffers

int cellular_get_modem_info(const struct device *dev, struct cellular_modem_info * modem_info);
{
        modem_info->imei = (const char *)data->imei;
}

This avoids a bunch of copying, and allows the driver to determine and manage the sizes of the members of struct cellular_modem_info

That said, I would prefer using a getter like int cellular_get_modem_info(enum modem_info_type type, char *info, size_t size); This is more flexible as we can add info to enum modem_info_type, without modifying the struct cellular_modem_info struct, and we can return -ENODATA if the modem does not provide said info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes perfect sense

Comment on lines 61 to 59
struct cellular_signal {
/* RSSI signal */
int16_t rssi;
/* Bit Error Rate */
uint8_t ber;
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think bit error rate should be used here as defined by the 3GPP specification at least, given that it is often excluded (set to 99) and is terribly complicated to decode
image

We should find a good way to abstract this data to be useful to the user IMO, RSSI is quite useful and is AFAIK always available, but even this could be abstracted into a EXCELLENT, GOOD, BAD spectrum, or a percentage, like what has been attempted here https://usatcorp.com/faqs/understanding-lte-signal-strength-values/?__cf_chl_tk=S8.MOHvJvlqFnfSdCNbPHzD_fq9sPTvCtPyve0O8FzI-1700753956-0-gaNycGzNC1A#section-2-1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you agree about the concept of having a separate API for static / pull once get_modem_info and for dynamic / pull several times get_signal ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, static modem info is quite limited right? IMEI and model number is pretty much the only non changing property I can think of which we have to poll from the modem, others depend on network selection, sim card inserted etc. Even properties like firmware versions can change during runtime if the modem is updated :)

I think overall the modem driver should determine how often it needs to read the static data, and the API should not care if the value is static or not, so I'm leaning towards a single API

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to suggest to add RSRP and RSRQ, the latter potentially replacing BER. RSRP is a better measurement of signal strength (relevant signal strength anyways), and commonly used in LTE.

Obviously if a modem is using GSM or 3G this might not be available, which should be indicated for example by setting RSRP/Q to INT16_MIN. But at least it should be there since 4G is the current and probably most used technology

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all values are integers, we could create a getter like the one for modem_info, which allows us to define both RSSI, RSRP and RSRQ in an enum, and return -ENODATA if the value is not present. I really like to avoid reserved values if possible :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSSI and RSRP are both in dBm which is quite nice, RSRQ seems to be dB which makes it a bit less nice to include if we use a getter for signal strength

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, static modem info is quite limited right? IMEI and model number is pretty much the only non changing property I can think of which we have to poll from the modem, others depend on network selection, sim card inserted etc. Even properties like firmware versions can change during runtime if the modem is updated :)

I think overall the modem driver should determine how often it needs to read the static data, and the API should not care if the value is static or not, so I'm leaning towards a single API

So far I also had in mind that there should be a separation between static (btw, I think these are IMEI, model number and manufacturer) and dynamic information. But @bjarki-trackunit has a good point. Things like the sim card and the fw version can change in runtime, so static information is very limited. It would be simpler to keep a single API for everything and in the end, it should be the application that decides what part of the data is static and what not.

On another note, is it actually the modem driver or the app that determines how often this info have to be read from the modem? Perhaps the time interval should be configurable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the application to request RSSI etc. as often as it sees fit, I don't think we actually need to store it and poll periodically

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the application to request RSSI etc. as often as it sees fit, I don't think we actually need to store it and poll periodically

Yes, that probably sounds the most rational thing to do.

@ldenefle ldenefle force-pushed the modem-read-rssi-upstream branch 3 times, most recently from 66aa74b to 58f3503 Compare November 24, 2023 09:18
@bjarki-trackunit
Copy link
Collaborator

@naNEQ This may be of interrest :)

@ldenefle ldenefle force-pushed the modem-read-rssi-upstream branch 3 times, most recently from 85c78d6 to 8e1c964 Compare November 28, 2023 16:10
@ldenefle
Copy link
Contributor Author

@bjarki-trackunit what do you think of current API ? I can add some demo code in the sample too

include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
drivers/modem/modem_cellular.c Outdated Show resolved Hide resolved
drivers/modem/modem_cellular.c Outdated Show resolved Hide resolved
drivers/modem/modem_cellular.c Show resolved Hide resolved
include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
samples/net/cellular_modem/src/main.c Show resolved Hide resolved
@ldenefle
Copy link
Contributor Author

ldenefle commented Dec 4, 2023

Hello, addressed all the changes requested, I'm testing them and pushing soon.

Adds two APIs which allow for configuring the cellular
network configuration of a cellular network device. like
a cellular modem. The first allows for configuring which
access technology to use, and optionally, which bands to
use. The second allows for getting all supported access
technologies are supported, and which bands for each tech
are supported.

Signed-off-by: Bjarki Arge Andreasen <bjarkix123@gmail.com>
@ldenefle ldenefle force-pushed the modem-read-rssi-upstream branch 2 times, most recently from 3d9a9ba to b88546d Compare December 4, 2023 18:44
drivers/modem/modem_cellular.c Outdated Show resolved Hide resolved
drivers/modem/modem_cellular.c Outdated Show resolved Hide resolved
drivers/modem/modem_cellular.c Outdated Show resolved Hide resolved
drivers/modem/modem_cellular.c Outdated Show resolved Hide resolved
drivers/modem/modem_cellular.c Outdated Show resolved Hide resolved
samples/net/cellular_modem/src/main.c Outdated Show resolved Hide resolved
jukkar
jukkar previously approved these changes Dec 8, 2023
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

rerickson1
rerickson1 previously approved these changes Dec 8, 2023
Copy link
Collaborator

@bjarki-trackunit bjarki-trackunit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
include/zephyr/drivers/cellular.h Outdated Show resolved Hide resolved
Implement modem info pulling using the init and periodic chat scripts

Signed-off-by: Lucas Denefle <lucas.denefle@converge.io>
Adds an example of polling cellular info to the modem sample

Signed-off-by: Lucas Denefle <lucas.denefle@converge.io>
@carlescufi carlescufi merged commit b287db8 into zephyrproject-rtos:main Dec 12, 2023
18 checks passed
@ldenefle ldenefle deleted the modem-read-rssi-upstream branch December 13, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants