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.

Sign up for GitHub

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

Jump to bottom

samples: Add modem shell sample. #4546

Merged
merged 6 commits into from
Jun 4, 2021
Merged

samples: Add modem shell sample. #4546

merged 6 commits into from
Jun 4, 2021

Conversation

trantanen
Copy link
Contributor

Adding modem shell sample with shell command line interface for
at commands, lte link control, sms, socket tool, ping, curl, iperf3.

@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label May 14, 2021
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

rlubos
rlubos reviewed May 20, 2021
View reviewed changes
@trantanen trantanen force-pushed the modem_shell branch 5 times, most recently from f7f4a29 to eefbe35 Compare May 20, 2021 13:16
@trantanen trantanen marked this pull request as ready for review May 21, 2021 07:58
peknis
peknis requested changes May 21, 2021
View reviewed changes
@trantanen trantanen force-pushed the modem_shell branch 2 times, most recently from 9074b2a to f5ef159 Compare May 24, 2021 09:30
rlubos
rlubos reviewed May 24, 2021
View reviewed changes
rlubos
rlubos reviewed May 24, 2021
View reviewed changes
rlubos
rlubos reviewed May 24, 2021
View reviewed changes
@rlubos
Copy link
Contributor

rlubos commented May 24, 2021

Up for discussion whether to keep all build-system related files for other platforms - autotools, all of these MakeFiles etc. But we should definitely drop the autotools generated files (Makefile, Makefile.in).

rlubos
rlubos reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Still half-way throguh (I hope).

rlubos
rlubos reviewed May 24, 2021
View reviewed changes
rlubos
rlubos reviewed May 25, 2021
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

Ok, I'm done with the first iteration, that was massive. Looks pretty good IMO, I've tried not to nitpick too much as there's simply too much code to do that.

lemrey
lemrey reviewed May 25, 2021
View reviewed changes
Copy link
Contributor

@lemrey lemrey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am doing a pass, it will take a bit so I'll start posting what I found so far, which is mainly:

  1. The patching to cURL, getopt and iperf3 is a bit inconsistent, there is a mix of commented out code, and ifdefs on several different symbols (CONFIG_MOSH_CURL_FUNCTIONAL_CHANGES, NOT_IN_MOSH_CURL_INTEGRATION, #if 0. It would be good to have some consistency to be able to grep differences between the vanilla implementation and our port across this three projects.
  2. Why is cURL is modem_shell and not under /ext like other getopt etc.?

@trantanen trantanen force-pushed the modem_shell branch 2 times, most recently from 5636363 to 334e3b8 Compare May 25, 2021 18:53
rlubos
rlubos approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@rlubos rlubos left a comment

Choose a reason for hiding this comment

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

No more issues from my side. Approving early as I'm OoO till the end of the week.

@trantanen trantanen force-pushed the modem_shell branch 4 times, most recently from 23ae3dd to 2a98370 Compare June 3, 2021 05:10
peknis
peknis requested changes Jun 3, 2021
View reviewed changes
@trantanen trantanen force-pushed the modem_shell branch 2 times, most recently from 67d5bc1 to a7ee1c6 Compare June 3, 2021 08:21
tejlmand
tejlmand reviewed Jun 3, 2021
View reviewed changes
tejlmand
tejlmand approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the work.

@lemrey
Copy link
Contributor

lemrey commented Jun 4, 2021

I would prefix the commits that add components to /ext with ext: ... and commits that add to /samples/nrf9160 with samples: nrf9160: ...

@trantanen
Copy link
Contributor Author

I would prefix the commits that add components to /ext with ext: ... and commits that add to /samples/nrf9160 with samples: nrf9160: ...

Done.

trantanen and others added 6 commits June 4, 2021 13:23
@trantanen
ext: iperf3: Add iperf3.
e33df84 
Adding iperf3 external libraries. Original copy has been taken from
https://github.com/esnet/iperf/commits/3.9 and modified slightly to
work in nrf context.
Signed-off-by: Jani Hirsimäki <jani.hirsimaki@nordicsemi.no>
Signed-off-by: Tommi Rantanen <tommi.rantanen@nordicsemi.no>
@trantanen
ext: getopt: Adding getopt.
c052ce2 
Adding FreeBSD getopt into external libraries.
Signed-off-by: Jani Hirsimäki <jani.hirsimaki@nordicsemi.no>
Signed-off-by: Tommi Rantanen <tommi.rantanen@nordicsemi.no>
@jhirsi @trantanen
lib: nrf_modem: zephyr packet raw sckt usage possibility not supported
4aeaa30 
AF_PACKET/SOCK_RAW/IPPROTO_RAW socket combo shall not be
handled by offloading socket if zephyr packet socket is built in.

Signed-off-by: Jani Hirsimäki <jani.hirsimaki@nordicsemi.no>
@trantanen
ext: curl: Add Curl.
39d88ff 
Adding Curl external library that has been copied from
https://github.com/curl/curl/tree/curl-7_73_0.
It has been modified quite a lot to work in nrf context.
Signed-off-by: Jani Hirsimäki <jani.hirsimaki@nordicsemi.no>
Signed-off-by: Tommi Rantanen <tommi.rantanen@nordicsemi.no>
@trantanen
doc: Add Infocenter link on UART interface
ebb55d4 
Add 'nRF9160 Hardware Verification Guidelines - UART interface' to
links.txt.
Signed-off-by: Tommi Rantanen <tommi.rantanen@nordicsemi.no>
@trantanen
samples: nrf9160: mosh: Add modem shell sample.
89fad77 
Adding modem shell sample with shell command line interface for
at commands, lte link control, sms, socket tool, ping, curl, iperf3.
Signed-off-by: Tommi Rantanen <tommi.rantanen@nordicsemi.no>
Signed-off-by: Jani Hirsimäki <jani.hirsimaki@nordicsemi.no>
Signed-off-by: Tommi Kangas <tommi.kangas@nordicsemi.no>
@carlescufi carlescufi merged commit e6ab4cc into nrfconnect:master Jun 4, 2021
@tejlmand tejlmand mentioned this pull request Jun 14, 2021
Merged
@trantanen trantanen deleted the modem_shell branch March 24, 2023 08:39
@trantanen trantanen restored the modem_shell branch March 24, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tokangas tokangas tokangas left review comments

@jtguggedal jtguggedal jtguggedal left review comments

@anangl anangl anangl left review comments

@jhirsi jhirsi jhirsi left review comments

@simensrostad simensrostad simensrostad left review comments

@lemrey lemrey lemrey approved these changes

@tejlmand tejlmand tejlmand approved these changes

@rlubos rlubos rlubos approved these changes

@peknis peknis peknis approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

Assignees
No one assigned
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.