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: Add cellular API for network configuration #64537

Conversation

bjarki-andreasen
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen commented Oct 29, 2023

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.

The specified radio access technologies are incomplete, and I think adding a "MAX" at the end to allow for vendor specific technologies may be appropriate, but that can be added any time in the future as well

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 <[email protected]>
@bjarki-andreasen bjarki-andreasen force-pushed the cellular_api_network_config branch from 40ef781 to cc4d82e Compare October 29, 2023 09:09
* List of bands, as defined by the specified cellular access technology,
* to enables. All supported bands are enabled if none are provided.
*/
uint16_t *bands;
Copy link
Member

Choose a reason for hiding this comment

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

This seem to mean that user or the API needs to allocate the band array from heap. Could we avoid that, what about setting an array here with max number of bands, or could we use a bitmask to define the bands?

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Oct 29, 2023

Choose a reason for hiding this comment

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

We will run into issues as there are band numbers that exceed 255 already n256 so we would need 5 uint64_t to cover it with a bitmask.

Usually, a user would only enable a few specific bands, and they are very specific to the region (available operators), so the list would not be that long if it is provided. Most will not provide the bands at all which is why it is optional :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you prefer setting enabling/disabling bands one at a time maybe?

Copy link
Member

Choose a reason for hiding this comment

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

I would just like to avoid using dynamic memory allocations here because of possible memory leaks.

About the band numbers, are they pre-defined? If so, then could you do

#define BAND_1 BIT(1)
#define BAND_2 BIT(2)
...

If not, then that is probably not an option.

If the number of bands is small, could we do instead?

struct cellular_network {
	/** Cellular access technology */
	enum cellular_access_technology technology;
	/**
	 * List of bands, as defined by the specified cellular access technology,
	 * to enables. All supported bands are enabled if none are provided.
	 */
	uint16_t bands[MAX_BANDS];
...

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Oct 30, 2023

Choose a reason for hiding this comment

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

The first option is not a possibility due to the number of bands per network technology :(

Regarding the fixed size array, I understand the concern regarding the potential memory unsafety, but it is not more unsafe than write(struct context *ctx, uint8_t *data, size_t size), which is a generally accepted pattern for non-fixed sized data. The bands do not need to be allocated on the heap, they can, and will most likely, be statically allocated or on the stack, even const, consider:

static const uint16_t lte_bands[] = {
        3,
        92,
        244,
};

static const struct cellular_network networks[] {
        {
                .technology = CELLULAR_ACCESS_TECHNOLOGY_LTE,
                .bands = lte_bands,
                .size = ARRAY_SIZE(lte_bands)
        },
        ...
};

The modems support a fixed set of technologies and bands, so these will be const and statically defined, which is why the
cellular_get_supported_networks function provides a pointer to a const networks array :) We can even create a macro to do this which increases safety:

CELLULAR_NETWORK_DEFINE(_name, _technology, ...)

which expands to

uint16_t lte_bands[] = {
        3,
        92,
        244,
};

struct cellular_network lte_network {
        .technology = CELLULAR_ACCESS_TECHNOLOGY_LTE,
        .bands = lte_bands,
        .size = ARRAY_SIZE(lte_bands)
};

the va args being the bands

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A fixed size array will result in wasted memory given it has to fit the largest set of bands, which is not desired :)

Copy link
Member

Choose a reason for hiding this comment

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

I might have missed how the api is used, could you add a sample or test to show the intended usage.

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Nov 1, 2023

Choose a reason for hiding this comment

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

The driver will create an array of supported networks based on the modem datasheet. The datasheet will provide a list like this

LTE-FDD: B1/2/3/4/5/8/12/13/18/19/20/26*/28;
EGPRS: 850/900/1800/1900MHz

Which will then be stored in ROM, within the driver, like:

static const uint16_t supported_lte_bands = {
        1, 2, 3, 4, 5, 8, 12, 13, 18, 19, 20, 26, 28,
};

static const uint16_t supported_gsm_bands = {
        800, 900, 1800, 1900,
};

static const struct cellular_network supported_networks[] = {
        {
                .technology = CELLULAR_ACCESS_TECHNOLOGY_LTE_CAT_M1,
                .bands = supported_lte_bands,
                .size = ARRAY_SIZE(supported_lte_bands),
        },
        {
                .technology = CELLULAR_ACCESS_TECHNOLOGY_LTE_NB_IOT,
                .bands = supported_lte_bands,
                .size = ARRAY_SIZE(supported_lte_bands),
       },
        {
                .technology = CELLULAR_ACCESS_TECHNOLOGY_GSM,
                .bands = supported_gsm_bands,
                .size = ARRAY_SIZE(supported_gsm_bands),
       },
};

The application can then get the pointer to this const array using the cellular_get_supported_networks() function. The supported bands are like flash pages, stored statically and most likely const, (at the very least initialized in the modem init function)

        const struct device modem = DEVICE_DT_GET(DT_ALIAS(modem));
        const struct cellular_network *networks;
        uint8_t networks_size;
        cellular_get_supported_networks(modem, &networks, &networks_size);

        /* */
        if (networks[0].technology == CELLULAR_ACCESS_TECHNOLOGY_LTE_CAT_M1) {
                ...

To configure the modem to use LTE_CAT_M1, and GSM only, allowing all bands,, the application will do

        const struct cellular_network networks[] = {
                {
                        .technology = CELLULAR_ACCESS_TECHNOLOGY_LTE_CAT_M1,
                },
                {
                        .technology = CELLULAR_ACCESS_TECHNOLOGY_GSM,
                }
        };
        cellular_configure_networks(modem, networks, ARRAY_SIZE(networks));        

In this case the networks only needs to be valid until the function returns, like when writing data for example

@ldenefle
Copy link
Contributor

Hello, I'd like to work on this PR, I need to be able to poll information from the cellular modem like rssi, imei and imsi.
Do you mind if I use your changes ?

@bjarki-andreasen
Copy link
Collaborator Author

bjarki-andreasen commented Nov 23, 2023

Hello, I'd like to work on this PR, I need to be able to poll information from the cellular modem like rssi, imei and imsi. Do you mind if I use your changes ?

Nice, go right ahead :) If you get a PR ready be sure to notify me so I can add it to the API progress overview #64405 :)

@bjarki-andreasen
Copy link
Collaborator Author

Was added in PR #65685 :)

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.

4 participants