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

tools: Add cmux command line tool #14823

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

Conversation

xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 commented Nov 16, 2024

Summary

Usage: ./nuttx/tools/cmux -d -n -b

Please refer to the following link for more information:
https://docs.kernel.org/6.1/driver-api/tty/n_gsm.html

Impact

new host tool

Testing

ci

@github-actions github-actions bot added Area: Tooling Size: M The size of the change in this PR is medium labels Nov 16, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 16, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing via CI, several crucial pieces of information are missing.

Here's a breakdown of what's lacking:

  • Insufficient Summary Detail: The summary provides basic usage but doesn't explain why this new tool is necessary. What problem does it solve? What existing functionality does it improve upon (or replace)? How does it work internally? Are there related NuttX issues?

  • Missing Impact Assessment: Simply stating "new host tool" is insufficient. Consider the impact points listed in the requirements:

    • Feature: Is it a new feature? Or does it replace/modify an existing one?
    • User Impact: How will users interact with this tool? Will existing workflows change?
    • Build Impact: Does adding this tool require changes to the NuttX build system? Does it introduce new dependencies?
    • Hardware Impact: Does this tool interact with hardware in any specific way? Is it architecture-specific?
    • Documentation: This new tool requires documentation explaining its purpose, usage, and options.
    • Security/Compatibility: These points haven't been addressed. Even if the impact is "NO," it should be explicitly stated.
  • Inadequate Testing Information: "ci" is not enough. While CI testing is important, the PR needs to show specific test results.

    • Build Host Details: Specify the OS, CPU architecture, and compiler version used for testing.
    • Target Details: Which architecture(s) and board configurations was this tested on? Was it tested on real hardware or a simulator?
    • Logs: The PR needs to include "before" and "after" logs demonstrating the tool's functionality and the problem it solves (or the improvement it delivers). Simply saying "ci" provides no evidence that the tool actually works.

In short, the PR needs to be significantly more detailed to meet the NuttX requirements. It needs to provide a thorough explanation of the changes, their impact, and clear evidence of testing.

@acassis
Copy link
Contributor

acassis commented Nov 17, 2024

@xiaoxiang781216 I think this application depends on CMUX support in the NuttX kernel, is there a CMUX driver coming to NuttX kernel?

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

@xiaoxiang781216 I was thinking this was CMUX support for NuttX, but seems like it has other purpose, probably to run on Linux side. Could you please explain the idea of this tool? Please create a usage() function explain the purpose of this tool.

@xiaoxiang781216
Copy link
Contributor Author

@xiaoxiang781216 I think this application depends on CMUX support in the NuttX kernel, is there a CMUX driver coming to NuttX kernel?

Yes, it will come in the next patch.

Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

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

Please could you add a brief explanation what is cmux and what it is for in the PR description and commit log? Thanks.

Usage: ./nuttx/tools/cmux -d <device> -n <number> -b <baudrate>

Please refer to the following link for more information:
https://docs.kernel.org/6.1/driver-api/tty/n_gsm.html

Signed-off-by: yinshengkai <[email protected]>
@xiaoxiang781216
Copy link
Contributor Author

@xiaoxiang781216 I was thinking this was CMUX support for NuttX, but seems like it has other purpose, probably to run on Linux side. Could you please explain the idea of this tool? Please create a usage() function explain the purpose of this tool.

already in the code:

static void usage(const char *progname)
{
  fprintf(stderr, "Usage: %s -d <device> -n <number> -b <baudrate>\n",
          progname);
  exit(EXIT_FAILURE);
}

@xiaoxiang781216
Copy link
Contributor Author

Please could you add a brief explanation what is cmux and what it is for in the PR description and commit log? Thanks.

Update, you can learn the concept and spec from:
https://docs.kernel.org/6.1/driver-api/tty/n_gsm.html

@acassis
Copy link
Contributor

acassis commented Nov 18, 2024

Please could you add a brief explanation what is cmux and what it is for in the PR description and commit log? Thanks.

Update, you can learn the concept and spec from: https://docs.kernel.org/6.1/driver-api/tty/n_gsm.html

Thank you Xiang! What is the purpose of this external too to NuttX, since NuttX doesn't have CMUX support yet?

I think this is the question I and everybody that find this program at nuttx/tools/ will ask. This application is for Linux, not for NuttX, correct?

@acassis
Copy link
Contributor

acassis commented Nov 18, 2024

@xiaoxiang781216 I think /me and @hartmannathan are asking some question: why to include a program that is supported only for Linux into NuttX? The documentation you provided is for Linux, please consider including a Documentation/ to explain the goal and purpose to have a Linux host CMUX GSM instead of adding CMUX support on NuttX and adding this cmux tool at nuttx-apps

@acassis
Copy link
Contributor

acassis commented Nov 18, 2024

@xiaoxiang781216 there is an issue already opened asking for CMUX support on NuttX: #7153

@acassis
Copy link
Contributor

acassis commented Nov 18, 2024

@xiaoxiang781216 I think this application depends on CMUX support in the NuttX kernel, is there a CMUX driver coming to NuttX kernel?

Yes, it will come in the next patch.

But in this case the cmux application should be inside nuttx-apps, instead of nuttx/tools/, right?

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Nov 18, 2024

Please could you add a brief explanation what is cmux and what it is for in the PR description and commit log? Thanks.

Update, you can learn the concept and spec from: https://docs.kernel.org/6.1/driver-api/tty/n_gsm.html

Thank you Xiang! What is the purpose of this external too to NuttX, since NuttX doesn't have CMUX support yet?

I think this is the question I and everybody that find this program at nuttx/tools/ will ask. This application is for Linux, not for NuttX, correct?

Yes, that's why this tool is put into nuttx/tools, not apps/.

@xiaoxiang781216 I think /me and @hartmannathan are asking some question: why to include a program that is supported only for Linux into NuttX?

Because you need this tool to change host serial port into mux mode.

The documentation you provided is for Linux, please consider including a Documentation/ to explain the goal and purpose to have a Linux host CMUX GSM

@Gary-Hobson will provide a NuttX serial driver which implement CMUX protocol.

instead of adding CMUX support on NuttX and adding this cmux tool at nuttx-apps

This tool doesn't need for NuttX since NuttX CMUX driver auto switch to mux mode by config, not through ioctl dynamically.

@xiaoxiang781216 I think this application depends on CMUX support in the NuttX kernel, is there a CMUX driver coming to NuttX kernel?

@xiaoxiang781216 there is an issue already opened asking for CMUX support on NuttX: #7153

Yes, after @Gary-Hobson upstream MUX serial driver, #7153 could be closed as complete.

Yes, it will come in the next patch.

But in this case the cmux application should be inside nuttx-apps, instead of nuttx/tools/, right?

No, NuttX mux driver could work automatically without the setup like Linux, that's why this tool put into nuttx/tools/ not apps/.

@acassis
Copy link
Contributor

acassis commented Nov 18, 2024

Please could you add a brief explanation what is cmux and what it is for in the PR description and commit log? Thanks.

Update, you can learn the concept and spec from: https://docs.kernel.org/6.1/driver-api/tty/n_gsm.html

Thank you Xiang! What is the purpose of this external too to NuttX, since NuttX doesn't have CMUX support yet?
I think this is the question I and everybody that find this program at nuttx/tools/ will ask. This application is for Linux, not for NuttX, correct?

Yes, that's why this tool is put into nuttx/tools, not apps/.

@xiaoxiang781216 I think /me and @hartmannathan are asking some question: why to include a program that is supported only for Linux into NuttX?

Because you need this tool to change host serial port into mux mode.

Why? Are you using the modem with SIM? Normally the CMUX will work integrated with PPP and the user should issue a command (or an IOCTL) to setup the CMUX (multiplexed) ports.

The documentation you provided is for Linux, please consider including a Documentation/ to explain the goal and purpose to have a Linux host CMUX GSM

@Gary-Hobson will provide a NuttX serial driver which implement CMUX protocol.

Great! Kudos @Gary-Hobson !

instead of adding CMUX support on NuttX and adding this cmux tool at nuttx-apps

This tool doesn't need for NuttX since NuttX CMUX driver auto switch to mux mode by config, not through ioctl dynamically.

Hmm, I think having IOCTL support could be useful in case where we want to enter / leave CMUX during the PPP execution.

@xiaoxiang781216 I think this application depends on CMUX support in the NuttX kernel, is there a CMUX driver coming to NuttX kernel?

@xiaoxiang781216 there is an issue already opened asking for CMUX support on NuttX: #7153

Yes, after @Gary-Hobson upstream MUX serial driver, #7153 could be closed as complete.

Yes, it will come in the next patch.

But in this case the cmux application should be inside nuttx-apps, instead of nuttx/tools/, right?

No, NuttX mux driver could work automatically without the setup like Linux, that's why this tool put into nuttx/tools/ not apps/.

Hmm, by automatically you mean statically, right? It should be use some board config to enter in this CMUX mode, correct?

@acassis
Copy link
Contributor

acassis commented Nov 18, 2024

@xiaoxiang781216 maybe you can include a verbose explanation inside this C file (just before "Included Files") explaining the purpose of this file inside nuttx/tools. This way it could help the absence of proper Documentation/ to this script

@xiaoxiang781216
Copy link
Contributor Author

Because you need this tool to change host serial port into mux mode.

Why? Are you using the modem with SIM?

We use cmux for debugging purpose to have the multiple virtual port for nsh/log/gdb at the same time on one physical uart.

Normally the CMUX will work integrated with PPP and the user should issue a command (or an IOCTL) to setup the CMUX (multiplexed) ports.

in this case, you can bypass cmux tool and do ioctl in your program.

This tool doesn't need for NuttX since NuttX CMUX driver auto switch to mux mode by config, not through ioctl dynamically.

Hmm, I think having IOCTL support could be useful in case where we want to enter / leave CMUX during the PPP execution.

it's hard to handle the enter/leave without the lost data, especially we want syslog can be worked as soon as possible in any environment.

No, NuttX mux driver could work automatically without the setup like Linux, that's why this tool put into nuttx/tools/ not apps/.

Hmm, by automatically you mean statically, right? It should be use some board config to enter in this CMUX mode, correct?

you can config through:

  1. Which physical port you want to use CMUX
  2. how many virtual serial ports you want

After initialization, you will see all virtual serial ports under /dev/ and use them like the normal uart. Of course, the other side(e.g. Linux) need config by their design as usually.

@Gary-Hobson
Copy link
Contributor

The nuttx kernel cmux driver support has been completed, but the internal review has not been completed (need to support sending and receiving data in interrupts).

Maybe I can upload the cmux driver in thread mode to the community for review tomorrow (does not support sending and receiving data in interrupts)

Currently, it has been verified that it can communicate with the linux kernel, and multiple gsmtty devices can be virtualized in the linux kernel (using cmux tools to configure linux), and the corresponding tty driver will also be generated in the nuttx kernel

PPP communication with modem is still to be verified

@acassis
Copy link
Contributor

acassis commented Nov 21, 2024

@xiaoxiang781216 please include an explanation in the source code or a Documentation/ about this script, to avoid having another script that nobody knows why it is inside nuttx


/* Create the actual character node */

if (mknod(devname, S_IFCHR | 0666, device) != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

@xiaoxiang781216 I think mknod will require root access, doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, cmux tool requires root privileges to execute

Copy link
Contributor

Choose a reason for hiding this comment

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

@Gary-Hobson thank you for confirming! @xiaoxiang781216 this information also should be included in the Documentation/ ! Please update the patch adding the documentation entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Tooling Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants