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

bring echo functionality on slm #18823

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TaeZStkyoht
Copy link

Modem Shell has already echo functionality on at_cmd_mode. Let's bring this functionality to Serial LTE Modem as well with by default false.

@TaeZStkyoht TaeZStkyoht requested review from a team as code owners November 12, 2024 15:57
@CLAassistant
Copy link

CLAassistant commented Nov 12, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 12, 2024
@NordicBuilder
Copy link
Contributor

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

@NordicBuilder NordicBuilder added the external External contribution label Nov 12, 2024
@@ -700,6 +700,10 @@ void slm_at_receive(const uint8_t *buf, size_t len)
break;
}

#if defined(CONFIG_SLM_UART_ECHO)
Copy link
Contributor

@MarkusLassila MarkusLassila Nov 13, 2024

Choose a reason for hiding this comment

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

SLM can handle multiple commands coming in a single UART buffer. And this loop is run once for each command. This printing would need to be moved out from the loop.

Copy link
Author

Choose a reason for hiding this comment

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

Now it's out from the loop.

@@ -700,6 +700,10 @@ void slm_at_receive(const uint8_t *buf, size_t len)
break;
}

#if defined(CONFIG_SLM_UART_ECHO)
slm_at_send(buf, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will print the data as well and should likely call slm_at_send_indicate without indicate and tracing.

Copy link
Author

Choose a reason for hiding this comment

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

Now it directly calls at_backend.send

@@ -68,6 +68,12 @@ config SLM_UART_TX_BUF_SIZE
help
Amount of UART traffic waiting to be sent (TX), that can be held. If the buffers are full, will send synchronously.

config SLM_UART_ECHO
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this deal with backspace?

Copy link
Author

Choose a reason for hiding this comment

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

Now it clears the line before putting buffer on it.

if (get_slm_mode() == SLM_AT_COMMAND_MODE && at_backend.send) {
const uint8_t clear_line_cmd[] = "\33[2K\r"; /* VT100 Clear line escape sequence */
at_backend.send(clear_line_cmd, sizeof(clear_line_cmd));
at_backend.send(slm_at_buf, at_cmd_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not work with multi-line inputs such as certificates with AT%CMNG commands. Only the last line is echoed for the terminal. Similarly, if the certificate is larger than UART buffers, the received parts will be repeated multiple times.

I also had problems with other multi-line inputs such as:
at#xsend="test
test"

The output buffer keeps on getting printed for every character on the second row.

Copy link
Contributor

@MarkusLassila MarkusLassila Nov 14, 2024

Choose a reason for hiding this comment

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

I am starting to think that this might be something that is quite hard to get to work correctly for all inputs (and terminals?). The local echo should handle this on terminals, so I don't fully understand the need for this. I am currently leaning on not approving this change to our main, even if it seems to work with most inputs.

Copy link
Author

Choose a reason for hiding this comment

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

I've fixed multi-line input problem with my last push.
I implemented this to not only for terminal usage but also if someone writes an application to talk with modem through uart and expects echo on the first response line for the parser function etc. Because Modem Shell (mosh) has it by default true.
First, I created a ticket on Nordic DevZone: https://devzone.nordicsemi.com/f/nordic-q-a/116126/serial-lte-modem-doesn-t-show-what-i-typed
And then I realized there is no such an option for this. That's the why I wanted to implement my own. And I implemented with by default false. Therefore, it won't affect the other people who use SLM.
By the way, I already arranged my parser. I don't need this change. Even I need it I can patch it. So, I don't need this PR to be merged personally. It may be helpful for other people in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is good that you got this to work in your parser. That is the way want to encourage our users to solve this issue. As for the integrations, ones that we currently have do not except echoes of commands. This need has not been risen during the time that I have worked with this component. I think that it may be more difficult to implement the parsing of responses if the original commands are echoed back.

If such need arises in future, this PR can be revisited.

For future reference, PR has the before mentioned issues:

  • Multi-line inputs keep repeating previous rows:
    at#xsend="test
    test"

  • Messages that are larger than the size of the UART buffers will be repeated multiple times.

There are sure to be other issues as well, but this will likely work well enough for some use-cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. external External contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants