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

modem: modem_cmux: Increase modem cmux buf size #63812

Conversation

bjarki-andreasen
Copy link
Collaborator

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

This PR fixes two issues preventing the modem_cellular.c driver from connecting and configuring CMUX for the SIMCOM SIM7080 modem.

  1. Increases the buffer used for commands in the control channel within an instance of the
    modem_cmux module. The buffer was not large enough to store an MSC command if the optional break signals where included. This commit fixes the issue and updates the test suite to use the max size MSC message.

  2. Adjust AT+CMUX chat script requests to only contain required parameters.

Fixes #63813

Bjarki Arge Andreasen added 2 commits October 11, 2023 15:51
This commit increases the buffer used for commands
in the control channel within an instance of the
modem_cmux module. The buffer was not large enough to
store an MSC command if the optional break signals
where included. This commit fixes the issue and
updates the test suite to use the max size MSC message.

Signed-off-by: Bjarki Arge Andreasen <baa@trackunit.com>
This commit adjust the chat scripts for the simcom
sim7080 and gsm_ppp compatibles to fix an issue found
when trying to use a sim7080 modem. The CMUX command
includes optional parameters which are not identical
for all modems, so the AT+CMUX command has been adjusted
to only contain the mandatory parameters.

Signed-off-by: Bjarki Arge Andreasen <baa@trackunit.com>
@fabiobaltieri fabiobaltieri added this to the v3.5.0 milestone Oct 11, 2023
Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

thanks!

@@ -1365,8 +1364,7 @@ MODEM_CHAT_SCRIPT_CMDS_DEFINE(simcom_sim7080_init_chat_script_cmds,
MODEM_CHAT_SCRIPT_CMD_RESP("AT+CGSN", imei_match),
MODEM_CHAT_SCRIPT_CMD_RESP("", ok_match),
MODEM_CHAT_SCRIPT_CMD_RESP("AT+CGMM", cgmm_match),
MODEM_CHAT_SCRIPT_CMD_RESP_NONE("AT+CMUX=0,0,5,127,10,3,30,10,2",
0));
MODEM_CHAT_SCRIPT_CMD_RESP_NONE("AT+CMUX=0,0,5,127", 300));
Copy link
Collaborator

Choose a reason for hiding this comment

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

non blocker: FWIW SIM7080 does support N1, T1, N2 and T2 options, and simply not T3 and K like BG95 has, and they are defaulting respectively to 118, 0, 0, 600. Would it make sense to stick to the below, or does it makes no useful difference?

Suggested change
MODEM_CHAT_SCRIPT_CMD_RESP_NONE("AT+CMUX=0,0,5,127", 300));
MODEM_CHAT_SCRIPT_CMD_RESP_NONE("AT+CMUX=0,0,5,127,10,3,30", 300));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The defaults are 0,0,600 as the maximum frame size is set to 127. The maximum frame size is important as the modem_cmux instance internal buffer is scaled to this value. The T1 and N2 are not used or not configurable for the SIM7080, so it really won't matter what we set them to. I would prefer them and T2 to be default.

@@ -23,7 +23,7 @@ LOG_MODULE_REGISTER(modem_cmux, CONFIG_MODEM_MODULES_LOG_LEVEL);
#define MODEM_CMUX_DATA_FRAME_SIZE_MIN (MODEM_CMUX_FRAME_SIZE_MAX + \
MODEM_CMUX_DATA_SIZE_MIN)

#define MODEM_CMUX_CMD_DATA_SIZE_MAX (0x04)
#define MODEM_CMUX_CMD_DATA_SIZE_MAX (0x08)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Confirmed as working. I had initially tested with 0x10 but 0x08 seems to be enough to get rid of the buffer overrun I was seeing otherwise.

Copy link
Collaborator Author

@bjarki-andreasen bjarki-andreasen Oct 11, 2023

Choose a reason for hiding this comment

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

Nice :) I replicated the exact data that was sent from your modem in the CMUX unit test which is why I decided to go with a smaller buffer with confidence :)

@bjarki-andreasen
Copy link
Collaborator Author

Thanks @kartben for retesting it!

@jhedberg jhedberg merged commit 0339f2d into zephyrproject-rtos:main Oct 11, 2023
21 checks passed
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.

Bug: modem_cellular.c fails to resume sim7080 modem
6 participants