modem: cellular: baudrate update from devicetree

[JordanYates]

Move baudrate updates from a global kconfig to a devicetree property.

Implemented by extending the modem chat API to support requests to be provided from a callback instead of being statically specified.

Tested on an OOT modem using the same implementation as applied for the LARA_R6.

[bjarki-andreasen]

Scripts are designed to be prepared in advance, they can be hardcoded to save RAM, or they can be stored in RAM and prepared at runtime.

[JordanYates]

Scripts are designed to be prepared in advance, they can be hardcoded to save RAM, or they can be stored in RAM and prepared at runtime.

And this PR was designed to preserve that capability to the greatest extent possible.
For the LARA_R6 driver, this PR doesn't result in any ROM strings moving to RAM, and only adds 4 bytes RAM overhead (the pointer in the data struct) and whatever the modem_baudrate_cmd command costs in ROM.

In return, this allows moving hardware configuration to devicetree, which is where it should be. More generally, it allows tailoring any AT commands which should be configured on a per device basis, rather than the hardcoded configuration currently.

The current behaviour of CONFIG_MODEM_CELLULAR_NEW_BAUDRATE makes little sense, Kconfig has no way of knowing which baudrates should be used for a given board, it is intrinsically tied to the hardware configuration.

[rerickson1]

Isn't baud rate dependent on host capabilities? Might a more conservative default of 1000000 be better?

[JordanYates]

Absolutely, and this was the primary motivation for the change. The value of 3000000 was chosen to match the deprecated kconfig option, but I can drop it back to 921600 or something if that is preferable.

[bjarki-andreasen]

Scripts are designed to be prepared in advance, they can be hardcoded to save RAM, or they can be stored in RAM and prepared at runtime.

And this PR was designed to preserve that capability to the greatest extent possible. For the LARA_R6 driver, this PR doesn't result in any ROM strings moving to RAM, and only adds 4 bytes RAM overhead (the pointer in the data struct) and whatever the modem_baudrate_cmd command costs in ROM.

Simplicity is the key here, not saving a few bytes. The simplest way to achieve this is to just define a custom baudrate request script for the instance of the lara_r6.

#define U_BLOX_LARA_R6_SET_BAUDRATE_CHAT_SCRIPT_DEFINE(inst)					\
	MODEM_CHAT_SCRIPT_CMDS_DEFINE(u_blox_lara_r6_set_baudrate_chat_script_cmds##inst,	\
		MODEM_CHAT_SCRIPT_CMD_RESP("ATE0", ok_match),					\
		MODEM_CHAT_SCRIPT_CMD_RESP(							\
			"AT+IPR=" STRINGIFY(DT_INST_PROP(inst, target_speed)),			\
			ok_match								\
		)										\
	);											\
												\
	MODEM_CHAT_SCRIPT_DEFINE(u_blox_lara_r6_set_baudrate_chat_script##inst,			\
		u_blox_lara_r6_set_baudrate_chat_script_cmds##inst,					\
		abort_matches,									\
		modem_cellular_chat_callback_handler,						\
		1										\
	);

...

	U_BLOX_LARA_R6_SET_BAUDRATE_CHAT_SCRIPT_DEFINE(inst)                                       \
                                                                                                \
	MODEM_CELLULAR_DEFINE_INSTANCE(inst, 1500, 100, 9000, 5000, false,                         \
				       &u_blox_lara_r6_set_baudrate_chat_script##inst,          \
				       &u_blox_lara_r6_init_chat_script,                           \
				       &u_blox_lara_r6_dial_chat_script,                           \
				       &u_blox_lara_r6_periodic_chat_script,                       \
				       NULL)

and if this is common to all modems, which it looks like it is, a helper can be created which is a bit more general.

A more complex way would be to add a mutable script and use that across all modems like we do for apn requests but those cost quite a bit of RAM and complexity.

In return, this allows moving hardware configuration to devicetree, which is where it should be. More generally, it allows tailoring any AT commands which should be configured on a per device basis, rather than the hardcoded configuration currently.

The current behaviour of CONFIG_MODEM_CELLULAR_NEW_BAUDRATE makes little sense, Kconfig has no way of knowing which baudrates should be used for a given board, it is intrinsically tied to the hardware configuration.

I agree with the problem, I just don't agree with the proposed solution

[JordanYates]

Simplicity is the key here

I guess the question is, simplicity for who, and where. Your proposal works, but basically doubles the size of the device creation macro. I am skeptical of turning it into a generic macro, if configuration could be generalized in that way, we probably wouldn't be duplicating every chat script for every modem.

The second optioned mentioned, the mutable scripts used by the APN configuration, is a ridiculous amount of boilerplate and code for what essentially boils down to sending a compile time const string.

But even ignoring those two points above, the changes to modem_chat enable a whole new class of functionality.

1. Query a configuration value in one step of a chat script (the result is stored in the data struct by the handler)
2. Send one of multiple commands in the next step of the script, depending on the previous result.

That's completely trivial with the ability to pull the string from a function, but I don't even want to think about the headache of doing it with dynamic scripts and flat out impossible with the constant scripts.

My follow up question would be, what part of this proposal has the complexity you are arguing against?
Because to be honest, I don't see it in either the extra branch inside modem_chat_send_script_request_part or the 14 line diff in modem_cellular.c.

[rerickson1]

I think this looks great. It's an elegant solution and provides an alternate way to send commands via modem chat.
Moving the baud rate to DT is also nice.

It's not clear to me where the issue lies here...

[JordanYates]

Removed CONFIG_MODEM_CELLULAR_NEW_BAUDRATE instead of deprecating it.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
12.4% Duplication on New Code

See analysis details on SonarQube Cloud

[bjarki-andreasen]

(sorry I accidentally edited the comment instead of quote replying)

Simplicity is the key here

I guess the question is, simplicity for who, and where.

Users of the modem subsystem, where complexity leads to bugs and makes it harder to work with.

Your proposal works, but basically doubles the size of the device creation macro. I am skeptical of turning it into a generic macro, if configuration could be generalized in that way, we probably wouldn't be duplicating every chat script for every modem.

The size of the macro is not an issue for me, it has no affect on code size, and could be made generic, thus making that size negligible.

The second optioned mentioned, the mutable scripts used by the APN configuration, is a ridiculous amount of boilerplate and code for what essentially boils down to sending a compile time const string.

Sure, I agree

But even ignoring those two points above, the changes to modem_chat enable a whole new class of functionality.

1. Query a configuration value in one step of a chat script (the result is stored in the data struct by the handler)
2. Send one of multiple commands in the next step of the script, depending on the previous result.

Functionality which I purposely designed against. AT like communication flow is predetermined, there is a known value to send before the script is started, and the expected response to the command, anything outside of that is an error. There should be no reason to be "in-flight" modifying the scripts behavior.

That's completely trivial with the ability to pull the string from a function, but I don't even want to think about the headache of doing it with dynamic scripts

We do exactly this in GNSS drivers, prepare script, send script, wait, repeat.

and flat out impossible with the constant scripts.

Yes, that's why we have dynamic ones

My follow up question would be, what part of this proposal has the complexity you are arguing against?
Because to be honest, I don't see it in either the extra branch inside modem_chat_send_script_request_part or the 14 line diff in modem_cellular.c.

The unnecessary changes to modem_chat is my biggest issue.

[JordanYates]

Users of the modem subsystem, where complexity leads to bugs and makes it harder to work with.

That's a blanket argument against adding any feature ever. I also disagree that the implementation details of the internals would somehow make the modem subsystem harder to work with.

Functionality which I purposely designed against. AT like communication flow is predetermined, there is a known value to send before the script is started, and the expected response to the command, anything outside of that is an error. There should be no reason to be "in-flight" modifying the scripts behavior.

That just seems unnecessarily limiting for no real reason. Are there complicated configuration chains that dynamic scripts are needed to solve, sure. Are there also chains which boil down to "if this send 1 else send 2", absolutely. I don't really understand why providing a simpler mechanism to solve the simpler problem is such a problem.

I would argue that forcing all users into the dynamic chat approach leads directly to "complexity leads to bugs and makes it harder to work with", which was said by someone at some point.

The unnecessary changes to modem_chat is my biggest issue.

So turning this:

	request_part = (uint8_t *)(&script_chat->request[chat->script_send_pos]);
	request_size = script_chat->request_size;

into this:

	if (script_chat->request_size == MODEM_CHAT_REQUEST_FN_LEN) {
		request_size = script_chat->request_fn(&request_part, chat->user_data);
		request_part += chat->script_send_pos;
	} else {
		request_part = (uint8_t *)(&script_chat->request[chat->script_send_pos]);
		request_size = script_chat->request_size;
	}

is an unacceptable increase in complexity? I am skeptical

[bjarki-andreasen]

Users of the modem subsystem, where complexity leads to bugs and makes it harder to work with.

That's a blanket argument against adding any feature ever. I also disagree that the implementation details of the internals would somehow make the modem subsystem harder to work with.

Its a rewording of KISS, which can be referred to as a blanket statement like any other foundational design philosophy.

Functionality which I purposely designed against. AT like communication flow is predetermined, there is a known value to send before the script is started, and the expected response to the command, anything outside of that is an error. There should be no reason to be "in-flight" modifying the scripts behavior.

That just seems unnecessarily limiting for no real reason. Are there complicated configuration chains that dynamic scripts are needed to solve, sure. Are there also chains which boil down to "if this send 1 else send 2", absolutely. I don't really understand why providing a simpler mechanism to solve the simpler problem is such a problem.

This is how I read the above: There is a solution which covers all static cases, and there is a solution that covers all dynamic cases, why can't we add a third solution which covers all static cases, and one more specific "dynamic" usecase.

"dynamic" being in quotes since its not actually a dynamic usecase, the string is known at compile time and immuatble.

I would argue that forcing all users into the dynamic chat approach leads directly to "complexity leads to bugs and makes it harder to work with", which was said by someone at some point.

The unnecessary changes to modem_chat is my biggest issue.

So turning this:

	request_part = (uint8_t *)(&script_chat->request[chat->script_send_pos]);
	request_size = script_chat->request_size;

into this:

	if (script_chat->request_size == MODEM_CHAT_REQUEST_FN_LEN) {
		request_size = script_chat->request_fn(&request_part, chat->user_data);
		request_part += chat->script_send_pos;
	} else {
		request_part = (uint8_t *)(&script_chat->request[chat->script_send_pos]);
		request_size = script_chat->request_size;
	}

is an unacceptable increase in complexity? I am skeptical

The dynamic scripts require more knowledge to use than the static ones, but with that knowledge, a user can modify everything. You may see "just a simple change to solve a specific issue", I see a series of follow up PRs adding a callback for the matches, then the response scripts, then the error responses... We already have a complete solution that works for all dynamic cases, a solution which is not even needed to solve the issue in this PR.

[JordanYates]

There is a solution which covers all static cases, and there is a solution that covers all dynamic cases, why can't we add a third solution which covers all static cases, and one more specific "dynamic" usecase.

Sure. I don't see why its such a problem to make solving a subclass of problems easier.
If you want to argue this doesn't make solving a class of problems easier, we're talking past each other.

You may see "just a simple change to solve a specific issue", I see a series of follow up PRs adding a callback for the matches, then the response scripts, then the error responses...

And if that makes solving a problem easier, whats your concern? Zephyr exists to solve problems.
If there was a theoretical fundamental problem with some PR implementing any of those, raise the concerns there.

[maass-hamburg]

generally fine, props that can change per dev instance, to have it in the dt, instead of a Kconfig

[bjarki-andreasen]

There is a solution which covers all static cases, and there is a solution that covers all dynamic cases, why can't we add a third solution which covers all static cases, and one more specific "dynamic" usecase.

Sure. I don't see why its such a problem to make solving a subclass of problems easier. If you want to argue this doesn't make solving a class of problems easier, we're talking past each other.

The third solution is not necessary nor scalable, thus it is not a proper solution to the problem.

You may see "just a simple change to solve a specific issue", I see a series of follow up PRs adding a callback for the matches, then the response scripts, then the error responses...

And if that makes solving a problem easier, whats your concern? Zephyr exists to solve problems. If there was a theoretical fundamental problem with some PR implementing any of those, raise the concerns there.

Like this PR? yeah, I did indeed raise the concern. The solution in this PR is not proper, a SPI transfer does not have a callback to change any of the buffers mid transfer, RTIO does not have a callback to modify an SQE mid transaction, modem_chat does not have a callback to modify a script mid transaction, I could go on. The entire transaction is known BEFORE the transaction is started, thus, that is when it shall be defined.