-
Notifications
You must be signed in to change notification settings - Fork 7.5k
samples: Bluetooth: hci_uart: Implement NOP Command Complete #14682
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
samples: Bluetooth: hci_uart: Implement NOP Command Complete #14682
Conversation
2931508
to
bfb4415
Compare
Codecov Report
@@ Coverage Diff @@
## master #14682 +/- ##
=======================================
Coverage 51.97% 51.97%
=======================================
Files 309 309
Lines 45584 45584
Branches 10555 10555
=======================================
Hits 23694 23694
Misses 17082 17082
Partials 4808 4808 Continue to review full report at Codecov.
|
bfb4415
to
d720395
Compare
This should address #15333. |
d720395
to
429aaab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally good, except for one minor endianness "correctness" issue
}, | ||
.cc = { | ||
.ncmd = 1, | ||
.opcode = BT_OP_NOP, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it makes no difference in practice, however this should really be sys_cpu_to_le16(BT_OP_NOP) to keep track of what endianness .opcode
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually allowed on an initializer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On big endian this maps to __bswap_16()
which is defined as follows:
#define __bswap_16(x) ((u16_t) ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8)))
I'd think the compiler should be able to handle that even for the initialiser of a const variable, but you could always try it by using __bswap_16(BT_OP_NOP)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. Updated, thanks!
429aaab
to
a173a09
Compare
}; | ||
|
||
for (i = 0; i < sizeof(cc_evt); i++) { | ||
uart_poll_out(hci_uart_dev, *(((u8_t *)&cc_evt)+i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(const u8_t *)
, just for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thank you
Implement the Controller counterpart to CONFIG_BT_WAIT_NOP so that it issues a NOP Command Complete event after booting up, to signal to the Host that it is ready to receive HCI traffic. Fixes zephyrproject-rtos#15333 Signed-off-by: Carles Cufi <[email protected]>
In order to fit in SRAM, reduce the CONFIG_BT_MAX_CONN from 20 to 16 in the configuration used for the BBC micro:bit. Signed-off-by: Carles Cufi <[email protected]>
9246da2
to
23f3fdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implement the Controller counterpart to CONFIG_BT_WAIT_NOP so that it
issues a NOP Command Complete event after booting up, to signal to the
Host that it is ready to receive HCI traffic.
Fixes #15333
Signed-off-by: Carles Cufi [email protected]