drivers: modem: gpio api and string len

[sslupsky]

This commit references modem_pin() and modem_shell()

modem_pin(): use new gpio api
The existing pin driver does not respect gpio
configuration in device tree for active high / low
This commit allows for the device tree to determine the
active logic level.
Fixes #23692

modem_shell(): use correct string length
The ms_send macro uses iface.write() to send a string.
iface.write() requires the length of the string not the
size of the string.
This commit corrects the string length.

[mike-scott]

@pabigot I forget why we used the raw API here?

[sslupsky]

Probably because the existing driver (sara-r4) has baked the logical state of the pin into the driver. That is, asserting the reset pin in the sara-r4 driver sets the pin to physical state 0 instead of a logical state 1.
This issue arose when I was looking at using the sara-r4 driver for the Arduino MKR NB 1500. On that board, there is an inversion of the signal between the mcu and the sara-r4 because of a level translator. So, the sara-r4 driver will not work without a modification. My goal is to make this a device tree setting for the board. That is, you specify whether the pin is active high or active low in the device tree, not the driver.

[jenmwms]

Can you please help me understand why this is changed from 2 to 1? I thought this string has 2 characters ("\r"), and the else statement next has a string of only 1 for a single space character. I must be missing something. Thanks!

[sslupsky]

\r is escaped to create a single character for the carriage return, so it is actually only one character even though it "looks" like it is two.

[sslupsky]

So, when you set the length to 2, you end up sending the carriage return and the null character. This generates an error from the modem (I am testing the MKR WAN 1310).

[pabigot]

This looks reasonable, but I would like to see a corresponding cleanup in the ublox-sara-r4 driver where this is used, specifically something like this (untested):

diff --git a/drivers/modem/ublox-sara-r4.c b/drivers/modem/ublox-sara-r4.c
index 590e761fbc..951370d40e 100644
--- a/drivers/modem/ublox-sara-r4.c
+++ b/drivers/modem/ublox-sara-r4.c
@@ -64,10 +64,6 @@ static struct modem_pin modem_pins[] = {
 #define MDM_POWER_DISABLE		0
 #define MDM_RESET_NOT_ASSERTED		1
 #define MDM_RESET_ASSERTED		0
-#if DT_INST_NODE_HAS_PROP(0, mdm_vint_gpios)
-#define MDM_VINT_ENABLE			1
-#define MDM_VINT_DISABLE		0
-#endif
 
 #define MDM_CMD_TIMEOUT			K_SECONDS(10)
 #define MDM_DNS_TIMEOUT			K_SECONDS(70)
@@ -642,7 +638,7 @@ static int pin_init(void)
 #if DT_INST_NODE_HAS_PROP(0, mdm_vint_gpios)
 	LOG_DBG("Waiting for MDM_VINT_PIN = 0");
 
-	while (modem_pin_read(&mctx, MDM_VINT) != MDM_VINT_DISABLE) {
+	while (modem_pin_read(&mctx, MDM_VINT) > 0) {
 #if defined(CONFIG_MODEM_UBLOX_SARA_U2)
 		/* try to power off again */
 		LOG_DBG("MDM_POWER_PIN -> DISABLE");
@@ -677,7 +673,7 @@ static int pin_init(void)
 	LOG_DBG("Waiting for MDM_VINT_PIN = 1");
 	do {
 		k_sleep(K_MSEC(100));
-	} while (modem_pin_read(&mctx, MDM_VINT) != MDM_VINT_ENABLE);
+	} while (modem_pin_read(&mctx, MDM_VINT) == 0);
 #else
 	k_sleep(K_SECONDS(10));
 #endif

This eliminates the implication that the enable and disable values can be something other than 1 and 0, and fixes the code so it won't enter an infinite loop if the GPIO read returns an error.

The code should still be updated to verify that an error wasn't returned, and to timeout if it doesn't power-on, but that's a bigger job.

As an aside: V_INT on this device appears to be a 1.8 V generated power rail. That should be documented in the devicetree YAML, as it seems possible to wire this up on a board where 1.8 V is below the high level threshold for a GPIO. Also not in scope for this PR, should be done with the extended changes suggested.

[WilliamGFish]

Just had a quick look at the Arduino range and noticed that the MKR NB 1500 is build around the Sara-R4 and Arduino MKR GSM 1400 the Sara-U2. With these changes the Arduino boards would have similar dts set-ups as the Particle ones.

The MKR WAN 1310 uses the SX1276 which also has support in Zephyr under Add initial LoRaWAN support #23664.

I will test these changes on alternative boards when I get a chance.

Billy..

[pabigot]

@sslupsky would you please evaluate my requested change so we can get this merged? Thanks.

[sslupsky]

@pabigot I should be able to do this today ... I've been distracted with some issues with the shell for the past few days.

@WilliamGFish Yes, that was the idea for the NB 1500. Regarding the MKR WAN 1310, this device uses a Murata module. So, you need to put the module in "passthrough" mode in order to make it look like a SX1276. I'm split between going that route and using the LoRaWAN stack in #23664 you referred to or utilizing the STM micro firmware on the module as Arduino does. The module firmware currently supports an AT command set. I have created a Zephyr POC driver that uses the AT command set to communicate with the module. I based it on the modem stack.

It does not appear ST plans to create a GitHub repo for the AT command firmware that resides on the module. The source code can be downloaded from ST's web site but having the community submit PR's is not possible.

There are a few design issues with the MKR WAN 1310 that need to be take into account to achieve the best power performance. Moreover, attention to memory footprint is necessary because of the limited SRAM (32K) of the SAMD21 MCU. Do you know what the SRAM memory requirements of the LoRaWAN #23644 are? I've got a pretty basic application running at the moment and I am pushing close to 30K of memory usage. So, if the LoRa stack consumes much RAM I am not sure it makes sense to implement the LoRa stack on the SAMD21.

What I think might be a better approach is to create a Zephyr application that runs on the Murata module using #23664 that replaces the existing module firmware. The module has an STM L0 or L4 MCU I believe (I cannot recall which one at the moment). This way, we could change the I/O interface between the SAMD21 and the module to use the SPI interface instead of the UART. This would also allow practical use of the 16 Mbit on board NOR flash that is on the MKR WAN 1310 now. At the moment, the nor flash is pretty much unusable because you need to put the module into reset to use the SPI bus to access the flash.

I was hoping to find someone else interested in MKR WAN 1310 to discuss these ideas. Let me know if you are interested to do so.

[sslupsky]

@pabigot I had a closer look at the sara-r4 driver and the data sheet.

First, I need to confirm that the pin enumerated as "MDM_POWER" in the driver corresponds to the pin described as "PWR_ON" in the datasheet (4.2.8). Do you know if that is that correct?

[sslupsky]

I downloaded the schematics for the Particle Boron which shows the PWR_ON connection. Just to confirm, the driver was originally developed for this board?

[sslupsky]

Your suggestion looks good. I cannot test it.

[sslupsky]

@pabigot ok, I pushed the change.

I believe you are correct, there are a few other things that need to be addressed in another PR.

[mike-scott]

Technically, this should probably be 2 commits, but I'm ok as is.

Thanks for submitting!

[sslupsky]

@mike-scott I think you are correct. I mistakenly thought all PR's had to be squashed into one commit. @pabigot clarified that for me.

[pabigot]

@mike-scott I think you are correct. I mistakenly thought all PR's had to be squashed into one commit. @pabigot clarified that for me.

Agreed. FWIW my breakdown is that the shell change gets its own commit, the change of the modem API to respect flags can be in one even though it touches both the generic API and a user of that API.