Bluetooth: shell: Port to the new shell #10239

Vudentz · 2018-09-26T14:03:26Z

This port the existing Bluetooth commands for the legacy shell to the new shell and modify the commands so protocols such as GATT, L2CAP, RFCOMM have their how subcommands instead of everything being under 'bt'.

The new hierarchy is as follow:

uart:~$ bt
bt - Bluetooth shell commands
Options:
-h, --help :Show command help.
Subcommands:
init :<address: XX:XX:XX:XX:XX:XX> <type: (public|random)>
hci-cmd : [data]
id-create :[addr]
id-reset : [addr]
id-delete :
id-show :[none]
id-select :
name :[name]
scan :<value: on, passive, off> <dup filter: dups, nodups>
advertise :<type: off, on, scan, nconn> <mode: discov, non_discov>
connect :<address: XX:XX:XX:XX:XX:XX> <type: (public|random)>
disconnect :[none]
auto-conn :<address: XX:XX:XX:XX:XX:XX> <type: (public|random)>
directed-adv :<address: XX:XX:XX:XX:XX:XX> <type: (public|random)>
[mode: low]
select :<address: XX:XX:XX:XX:XX:XX> <type: (public|random)>
conn-update :
oob :
clear :
channel-map :<channel-map: XX:XX:XX:XX:XX> (36-0)
security :<security level: 0, 1, 2, 3>
bondable :<bondable: on, off>
auth :<auth method: all, input, display, yesno, confirm,
none>
auth-cancel :[none]
auth-passkey :
auth-passkey-confirm :[none]
auth-pairing-confirm :[none]

uart:~$ br
br - Bluetooth BR/EDR shell commands
Options:
-h, --help :Show command help.
Subcommands:
auth-pincode :
connect :

discovery :<value: on, off> [length: 1-48] [mode: limited]
iscan :<value: on, off>
l2cap-register :
oob :
pscan :value: on, off
sdp-find :

uart:~$ gatt
gatt - Bluetooth GATT shell commands
Options:
-h, --help :Show command help.
Subcommands:
discover-characteristic :[UUID] [start handle] [end handle]
discover-descriptor :[UUID] [start handle] [end handle]
discover-include :[UUID] [start handle] [end handle]
discover-primary :[UUID] [start handle] [end handle]
discover-secondary :[UUID] [start handle] [end handle]
exchange-mtu :[none]
read : [offset]
read-multiple :<handle 1> <handle 2> ...
subscribe : [ind]
write : [length]
write-without-response : [length] [repeat]
write-signed : [length] [repeat]
unsubscribe :[none]
metrics :register vendr char and measure rx [value on, off]
register :register pre-predefined test service
show-db :[none]
unregister :unregister pre-predefined test service

uart:~$ l2cap
l2cap - Bluetooth L2CAP shell commands
Options:
-h, --help :Show command help.
Subcommands:
connect :
disconnect :[none]
metrics :<value on, off>
recv :[delay (in miliseconds)
register : [sec_level]
send :

uart:~$ rfcomm
rfcomm - Bluetooth RFCOMM shell commands
Options:
-h, --help :Show command help.
Subcommands:
register :
connect :
disconnect :[none]
send :

codecov-io · 2018-09-26T14:31:50Z

Codecov Report

Merging #10239 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10239      +/-   ##
==========================================
- Coverage   53.05%   53.04%   -0.01%     
==========================================
  Files         214      214              
  Lines       26201    26229      +28     
  Branches     5645     5653       +8     
==========================================
+ Hits        13901    13914      +13     
- Misses      10028    10042      +14     
- Partials     2272     2273       +1

Impacted Files	Coverage Δ
drivers/ethernet/eth_native_posix_adapt.c	`41.97% <0%> (+2.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10c6a0c...3ebfec7. Read the comment docs.

jakub-uC · 2018-09-26T15:12:46Z

subsys/bluetooth/shell/gatt.c

 {
 	int err;

 	if (!default_conn) {
-		printk("Not connected\n");
-		return 0;
+		print(shell, "Not connected\n");


New shell expects \r\n

jakub-uC · 2018-09-26T15:12:59Z

subsys/bluetooth/shell/gatt.c

 	}

 	exchange_params.func = exchange_func;

 	err = bt_gatt_exchange_mtu(default_conn, &exchange_params);
 	if (err) {
-		printk("Exchange failed (err %d)\n", err);
+		print(shell, "Exchange failed (err %d)\n", err);


jakub-uC · 2018-09-26T15:13:42Z

subsys/bluetooth/shell/gatt.c

 	}

-	printk("\n");
+	print(shell, "\n");


jakub-uC · 2018-09-26T15:13:48Z

subsys/bluetooth/shell/gatt.c

@@ -109,7 +108,7 @@ static u8_t discover_func(struct bt_conn *conn,
 	char str[37];

 	if (!attr) {
-		printk("Discover complete\n");
+		print(NULL, "Discover complete\n");


jakub-uC · 2018-09-26T15:19:55Z

subsys/bluetooth/shell/l2cap.c

+SHELL_CREATE_STATIC_SUBCMD_SET(l2cap_cmds) {
+	SHELL_CMD(connect, NULL, "<psm>", cmd_connect),
+	SHELL_CMD(disconnect, NULL, HELP_NONE, cmd_disconnect),
+	SHELL_CMD(metrics, NULL, "<value on, off>", cmd_metrics),


You can consider implementing subcommands: off and on without a handler. In such case body of function cmd_metrics remain unchanged but off and on can be prompted and autocompleated.

You can also implement handlers to on and off. Code will look like:

static void cmd_metrics_on(const struct shell *shell, size_t argc, char *argv[]) { if (!shell_cmd_precheck(shell, argc == 1, NULL, 0)) { return; } l2cap_ops.recv = l2cap_recv_metrics; } static void cmd_metrics_off(const struct shell *shell, size_t argc, char *argv[]) { if (!shell_cmd_precheck(shell, argc == 1, NULL, 0)) { return; } l2cap_ops.recv = l2cap_recv; } static void cmd_metrics(const struct shell *shell, size_t argc, char *argv[]) { if (!shell_cmd_precheck(shell, argc == 1, NULL, 0)) { return; } print(shell, "Please specify valid argument.\r\n"); } SHELL_CREATE_STATIC_SUBCMD_SET(sub_cmd_connect) { SHELL_CMD(off, NULL, "switch off", cmd_metrics_off), SHELL_CMD(on, NULL, "switch on", cmd_metrics_on), SHELL_SUBCMD_SET_END };

and modify this line:
SHELL_CMD(metrics, NULL, "<value on, off>", cmd_metrics),
to
SHELL_CMD(metrics, &sub_cmd_connect, "switch value", cmd_metrics),

This approach will limit strcmp calls and simplify commands usage.

I didn't like the idea of going more than 2 subcommand, specially when all it would do is to show on/off to auto complete. Btw, I wish we didn't have to do the precheck ourselves so how about we define a format define parameters in the command description so the shell parse that to know how many parameters the command takes and do the precheck before calling the command handler. While at that we could also make it auto complete using the parameters not only commands (so we don't have to add command representing parameters).

Fair enough.

Regarding auto precheck I have some doubts. You will always have the same behavior for all commands when wrong argument count is passed.
What if in this case some people want to print error message while others command help.

Another problem I see is with command options defined with macro SHELL_OPT in command handler - see below example:

static void cmd_typical_handler(const struct shell *shell, size_t argc, char **argv) { /* Dummy option showing options usage */ static const struct shell_getopt_option opt[] = { SHELL_OPT( "--test", "-t", "test option help string" ), SHELL_OPT( "--dummy", "-d", "dummy option help string" ) }; /* Function shell_cmd_precheck will do one of below actions: * 1. print help if user will call it -h or --help parameter * 2. print error message if called with too many arguments (argc > 2) * * Each of these actions can be deactivated in Kconfig. */ if (!shell_cmd_precheck(shell, (argc <= 2), opt, sizeof(opt)/sizeof(opt[1]))) { return; } }

It will be hard to access these options from the shell context to print them in help message.

jakub-uC · 2018-09-26T15:31:01Z

General remak:

New shell expects \r\n to move to beginning of new line.
You can simplify some subcommands by adding one more level of subcommands - I provided one example in the review. This is not must be.

Vudentz · 2018-09-26T17:51:47Z

Will fix the \r\n, though that is very window-ish and Im pretty sure it worked with just \n for me so I would expect more people to make such mistake. I guess this is the problem of having shell_fprintf only function, perhaps having some like shell_print that adds whatever line termination the shell expects so the handlers don't have to use shell_fprintf directly.

Vudentz · 2018-09-27T20:16:00Z

recheck

Vudentz · 2018-09-28T06:16:54Z

@aescolar Looks like Im still not able to use the native board with shell or Im missing something?

aescolar · 2018-09-28T07:04:06Z

@Vudentz

warning: SHELL (defined at subsys/shell/Kconfig:37) has direct dependencies SERIAL with value n,

This is the problem. Those 2 project files[1] need to set CONFIG_SERIAL=y so the serial driver is compiled in.
[1] tests/bluetooth/shell/

Fixes zephyrproject-rtos#8873 Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This splits BR/EDR command under 'bt' to 'br' to reduce the command list and at same time cleaning the mix between different bearers. In addition to that also remove "br-" prefix for the commands: br - Bluetooth BR/EDR shell commands Options: -h, --help :Show command help. Subcommands: auth-pincode :<pincode> connect :<address> discovery :<value: on, off> [length: 1-48] [mode: limited] iscan :<value: on, off> l2cap-register :<psm> oob : pscan :value: on, off sdp-find :<HFPAG> rfcomm-register :<channel> rfcomm-connect :<channel> rfcomm-send :<number of packets> rfcomm-disconnect :[none] Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This splits RFCOMM command under 'br' to 'rfcomm' removing 'rfcomm-' prefix from commands: rfcomm - Bluetooth RFCOMM shell commands Options: -h, --help :Show command help. Subcommands: register :<channel> connect :<channel> disconnect :[none] send :<number of packets> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This splits GATT command under 'bt' to 'gatt' removing 'gatt-' prefix from commands: gatt - Bluetooth GATT shell commands Options: -h, --help :Show command help. Subcommands: discover-characteristic :[UUID] [start handle] [end handle] discover-descriptor :[UUID] [start handle] [end handle] discover-include :[UUID] [start handle] [end handle] discover-primary :[UUID] [start handle] [end handle] discover-secondary :[UUID] [start handle] [end handle] exchange-mtu :[none] read :<handle> [offset] read-multiple :<handle 1> <handle 2> ... subscribe :<CCC handle> <value handle> [ind] write :<handle> <offset> <data> [length] write-without-response :<handle> <data> [length] [repeat] write-signed :<handle> <data> [length] [repeat] unsubscribe :[none] metrics :register vendr char and measure rx [value on, off] register :register pre-predefined test service show-db :[none] unregister :unregister pre-predefined test service Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This splits L2CAP commands under 'bt' to 'l2cap' removing 'l2cap-' prefix from the commands: l2cap - Bluetooth L2CAP shell commands Options: -h, --help :Show command help. Subcommands: connect :<psm> disconnect :[none] metrics :<value on, off> recv :[delay (in miliseconds) register :<psm> [sec_level] send :<number of packets> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Shell expects each printed lined to be terminated with \r\n. Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Vudentz · 2018-09-28T10:12:08Z

recheck

Vudentz · 2018-09-28T13:52:48Z

@jarz-nordic any feedback on these? We can rework some of the commands later if we find it is worth adding more depth for the getting autocomplete.

jakub-uC · 2018-09-28T13:59:04Z

Did you change \n to \r\n?
On putty (Windows) i observed that cursor is not automatically execute carriage return.

I will verify it in 2h, it is hard on a phone :)

Vudentz · 2018-09-28T14:20:39Z

@jarz-nordic It should be part of the last patch:

3ebfec7#diff-606a62870ea9628b2da31a759b417a3e

jakub-uC · 2018-09-28T15:54:43Z

LGTM

Vudentz requested review from jhedberg, sjanc and tarunkum as code owners September 26, 2018 14:03

carlescufi requested a review from jakub-uC September 26, 2018 14:06

jakub-uC requested changes Sep 26, 2018

View reviewed changes

Vudentz force-pushed the bluetooth branch 2 times, most recently from 4d57b94 to 6fa3747 Compare September 27, 2018 11:32

Vudentz mentioned this pull request Sep 27, 2018

UART for POSIX_ARCH #10212

Merged

Vudentz force-pushed the bluetooth branch 2 times, most recently from 5ad805f to 47438bc Compare September 28, 2018 04:42

Vudentz added 6 commits September 28, 2018 11:02

Bluetooth: shell: Port commands to the news shell

b8e33e0

Fixes zephyrproject-rtos#8873 Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Bluetooth: shell: Use \r\n to terminate line

3ebfec7

Shell expects each printed lined to be terminated with \r\n. Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Vudentz force-pushed the bluetooth branch from 47438bc to 3ebfec7 Compare September 28, 2018 08:03

jakub-uC self-requested a review September 28, 2018 15:53

jakub-uC approved these changes Sep 28, 2018

View reviewed changes

Vudentz requested a review from carlescufi October 1, 2018 04:55

carlescufi approved these changes Oct 1, 2018

View reviewed changes

carlescufi merged commit 17558c9 into zephyrproject-rtos:master Oct 1, 2018

nordic-krch mentioned this pull request Oct 2, 2018

New shell subsystem #8872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: shell: Port to the new shell #10239

Bluetooth: shell: Port to the new shell #10239

Vudentz commented Sep 26, 2018

codecov-io commented Sep 26, 2018 •

edited

Loading

jakub-uC Sep 26, 2018

jakub-uC Sep 26, 2018

jakub-uC Sep 26, 2018

jakub-uC Sep 26, 2018

jakub-uC Sep 26, 2018 •

edited

Loading

Vudentz Sep 26, 2018

jakub-uC Sep 27, 2018

jakub-uC commented Sep 26, 2018

Vudentz commented Sep 26, 2018

Vudentz commented Sep 27, 2018

Vudentz commented Sep 28, 2018

aescolar commented Sep 28, 2018 •

edited

Loading

Vudentz commented Sep 28, 2018

Vudentz commented Sep 28, 2018

jakub-uC commented Sep 28, 2018 •

edited

Loading

Vudentz commented Sep 28, 2018

jakub-uC commented Sep 28, 2018

Bluetooth: shell: Port to the new shell #10239

Bluetooth: shell: Port to the new shell #10239

Conversation

Vudentz commented Sep 26, 2018

codecov-io commented Sep 26, 2018 • edited Loading

Codecov Report

jakub-uC Sep 26, 2018

Choose a reason for hiding this comment

jakub-uC Sep 26, 2018

Choose a reason for hiding this comment

jakub-uC Sep 26, 2018

Choose a reason for hiding this comment

jakub-uC Sep 26, 2018

Choose a reason for hiding this comment

jakub-uC Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Vudentz Sep 26, 2018

Choose a reason for hiding this comment

jakub-uC Sep 27, 2018

Choose a reason for hiding this comment

jakub-uC commented Sep 26, 2018

Vudentz commented Sep 26, 2018

Vudentz commented Sep 27, 2018

Vudentz commented Sep 28, 2018

aescolar commented Sep 28, 2018 • edited Loading

Vudentz commented Sep 28, 2018

Vudentz commented Sep 28, 2018

jakub-uC commented Sep 28, 2018 • edited Loading

Vudentz commented Sep 28, 2018

jakub-uC commented Sep 28, 2018

codecov-io commented Sep 26, 2018 •

edited

Loading

jakub-uC Sep 26, 2018 •

edited

Loading

aescolar commented Sep 28, 2018 •

edited

Loading

jakub-uC commented Sep 28, 2018 •

edited

Loading