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

[shell] move common shell commands to core library #6765

Merged
merged 1 commit into from
May 17, 2021

Conversation

gjc13
Copy link
Contributor

@gjc13 gjc13 commented May 13, 2021

Problem

We'd like to have a user-friendly CLI in the devices for developing and testing as we have in OpenThread.

Summary of Changes

This is the first PR of the effort to add shell as an optional component for all devices.
Main changes include:

  • Move wifi, ble and device config command to common library
  • The wifi commands has been simplified with less modes.
    Platform-specific commands have been removed.
  • The btp command is renamed to ble with all the not-implemented commands removed.
  • The device config command is renamed to config with unused configs removed.
  • Fix shell formatting on ESP32.

@todo
Copy link

todo bot commented May 13, 2021

Enable wifi network should be done by ConnectivityManager. (Or other platform neutral interfaces)

// TODO: Enable wifi network should be done by ConnectivityManager. (Or other platform neutral interfaces)
#if defined(CHIP_DEVICE_LAYER_TARGET)
#define DEVICENETWORKPROVISIONING_HEADER <platform/CHIP_DEVICE_LAYER_TARGET/DeviceNetworkProvisioningDelegateImpl.h>
#include DEVICENETWORKPROVISIONING_HEADER
#endif
using chip::DeviceLayer::ConnectivityManager;
using chip::DeviceLayer::ConnectivityMgr;
namespace chip {
namespace Shell {


This comment was generated by todo based on a TODO comment in 043bb68 in #6765. cc @gjc13.

@gjc13 gjc13 force-pushed the common-shell-commands branch from 043bb68 to 7a9b020 Compare May 13, 2021 04:06
@gjc13 gjc13 changed the title [shell] move common shell commands to common library [shell] move common shell commands to core library May 13, 2021
@gjc13 gjc13 force-pushed the common-shell-commands branch from 7a9b020 to b8a4c5c Compare May 13, 2021 05:39
Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Please rename code to NOT use names like helper/support/util/tools/core.

These names do not describe what the contents of the file generally is and end up being filled with unrelated parts of code as a result.

shell/shell_core.h stutters and if I remove the stutter I get shell/core.h which is unclear about what that is.

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Would also avoid 'common' in names.

examples/lock-app/esp32/.gitignore Outdated Show resolved Hide resolved
examples/shell/esp32/main/main.cpp Outdated Show resolved Hide resolved
src/lib/shell/commands/CommandBase64.cpp Outdated Show resolved Hide resolved
src/lib/shell/commands/CommandBase64.cpp Outdated Show resolved Hide resolved
@woody-apple
Copy link
Contributor

/rebase

@gjc13 gjc13 force-pushed the common-shell-commands branch 2 times, most recently from bebb1b8 to c61d6ec Compare May 14, 2021 03:34
@gjc13 gjc13 requested a review from andy31415 May 14, 2021 03:34
@gjc13 gjc13 force-pushed the common-shell-commands branch 2 times, most recently from f990daf to 3137f8b Compare May 14, 2021 05:48
@gjc13
Copy link
Contributor Author

gjc13 commented May 17, 2021

@andy31415 PTAL

@gjc13 gjc13 force-pushed the common-shell-commands branch from 3137f8b to 740cb69 Compare May 17, 2021 04:10
@boring-cyborg boring-cyborg bot added the config label May 17, 2021
@gjc13 gjc13 force-pushed the common-shell-commands branch from 740cb69 to 196e840 Compare May 17, 2021 05:29
Copy link
Contributor

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Note that we had renamed shell/shell.h to shell/shell_core.h to fix filename collision with Zephyr-supplied shell/shell.h. Now that the file was renamed to Shell.h the issue is gone on Linux/mac, but may still appear on case-insensitive systems like Windows. Can we possibly rename it to something like Engine.h? @andy31415 would you approve such a name? :)

examples/shell/standalone/main.cpp Outdated Show resolved Hide resolved
This is the first PR of the effort toadd shell as an optional component
for all devices.

* Move wifi, ble and device config command to common library
* The `wifi` commands has been simplified with less modes.
  Platform-specific commands have been removed.
* The `btp` command is renamed to `ble` with all the not-implemented
  commands removed.
* The `device config` command is renamed to `config` with unused configs
  removed.
* Fix shell formatting on ESP32.
@gjc13 gjc13 force-pushed the common-shell-commands branch from 196e840 to 8267f99 Compare May 17, 2021 11:49
@gjc13 gjc13 requested a review from Damian-Nordic May 17, 2021 11:49
@gjc13
Copy link
Contributor Author

gjc13 commented May 17, 2021

@Damian-Nordic renamed.

Copy link
Contributor

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Thank you!

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 8a51de2

File Section File VM
chip-shell.elf datas -8 -8
chip-shell.elf device_handles -12 -12
chip-shell.elf bss 0 -96
chip-shell.elf rodata -3240 -3240
chip-shell.elf text -3428 -3428
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,24430
.debug_line,0,4228
.debug_abbrev,0,4198
.shstrtab,0,-3
datas,-8,-8
device_handles,-12,-12
.debug_aranges,0,-56
bss,-96,0
.debug_frame,0,-416
.debug_ranges,0,-520
.debug_str,0,-682
.symtab,0,-2496
rodata,-3240,-3240
text,-3428,-3428
.strtab,0,-3545
.debug_loc,0,-4438


@andy31415
Copy link
Contributor

Note that we had renamed shell/shell.h to shell/shell_core.h to fix filename collision with Zephyr-supplied shell/shell.h. Now that the file was renamed to Shell.h the issue is gone on Linux/mac, but may still appear on case-insensitive systems like Windows. Can we possibly rename it to something like Engine.h? @andy31415 would you approve such a name? :)

Sure. Also my request to avoid odd names is not a ban, just trying hard. If there is a naming clash then that seems to warrant a name change. Would not block PR over this for sure.

@andy31415 andy31415 merged commit a9b8ade into project-chip:master May 17, 2021
pan- added a commit to ARMmbed/connectedhomeip that referenced this pull request May 17, 2021
pan- added a commit to pan-/connectedhomeip that referenced this pull request Jun 15, 2021
pan- added a commit to pan-/connectedhomeip that referenced this pull request Jun 18, 2021
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.

5 participants