-
Notifications
You must be signed in to change notification settings - Fork 9
Some fixes for Zephyr testing #39
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the changes :)
Can you elaborate on this ? We did not have a lot of testing done on this part and I am not surprised that issues remain. |
| OpCodeIndex::READ_AUTHENTICATED_PAYLOAD_TIMEOUT}, | ||
| {OpCode::WRITE_AUTHENTICATED_PAYLOAD_TIMEOUT, | ||
| OpCodeIndex::WRITE_AUTHENTICATED_PAYLOAD_TIMEOUT}, | ||
| // {OpCode::WRITE_AUTHENTICATED_PAYLOAD_TIMEOUT, |
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.
This is not the right place to disable the command; there are two options (aside from fixing the implementation...):
- in the list of hci_command_handlers_ below (but rootcanal will assert if the command is invoked)
- in the controller properties
model/controller/controller_properties.h, in the supported command mask
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've got no clear understanding of this command from reading Zephyr and other open source code. I'm not a SIG member (you probably are via Google), so I only use implementations as reference.
Disabled it the good way now
| kNumCommandPackets, ErrorCode::SUCCESS)); | ||
| } | ||
|
|
||
| void DualModeController::SetControllerToHostFlowControl(CommandView command) { |
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.
Can I ask you to implement host flow control ?
If you only added the command because zephyr was invoking it, another option would be to remove the command Set Controller to Host Flow Control from the supported command mask in the controller properties.
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 found that I should just apply the same CONFIG_BT_HCI_ACL_FLOW_CONTROL=n as for Android NetSim (supposedly the same Rootcanal?) to the samples. Discarded that commit now
Will make sure I document it when I write a doc for running Zephyr with Rootcanal for Zephyr docs
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 think removing it from supported_commands (here's the ID: https://github.com/google/bumble/blob/cce2e4d4e3238d8aa65e4ee2eefe2f13ab7f8dc0/bumble/hci.py#L1007) should also be done: no reason to report unsupported command as supported (or is it for Android or other bt stack that wants it supported but won't call?)
|
#40 syncs with AOSP and brings some ISO fixes. |
Make clangd work
Thank you! Rebased and now more examples work! |
Remove some unsupported commands which confuse Zephyr samples and cause them to run into assertions. Sync status from model/controller/dual_mode_controller.cc to model/controller/controller_properties.cc, to prevent host sending commands that make controller assert.
|
Okay, thanks for pointing me in the right direction and sorry for a long wait, I synchronized the command list to avoid misreporting those as supported. Now Zephyr warns me to set proper options: Please review if any of those disable actions break anything on your side. My help however is limited here, as I am a student learning BLE, and not a SIG member, so I only have limited information Here's the diff of supported commands (received by Bumble), most of them seem to already say Details10d9
< HCI_ENABLE_DEVICE_UNDER_TEST_MODE_COMMAND
16d14
< HCI_FLUSH_COMMAND
19d16
< HCI_HOST_NUMBER_OF_COMPLETED_PACKETS_COMMAND
48d44
< HCI_LE_READ_CHANNEL_MAP_COMMAND
61d56
< HCI_LE_READ_RF_PATH_COMPENSATION_COMMAND
64d58
< HCI_LE_RECEIVER_TEST_COMMAND
91d84
< HCI_LE_SET_HOST_CHANNEL_CLASSIFICATION_COMMAND
104,106d96
< HCI_LE_TEST_END_COMMAND
< HCI_LE_TRANSMITTER_TEST_COMMAND
< HCI_LE_WRITE_RF_PATH_COMPENSATION_COMMAND
114,116d103
< HCI_READ_AFH_CHANNEL_ASSESSMENT_MODE_COMMAND
< HCI_READ_AFH_CHANNEL_MAP_COMMAND
< HCI_READ_AUTHENTICATED_PAYLOAD_TIMEOUT_COMMAND
118d104
< HCI_READ_AUTOMATIC_FLUSH_TIMEOUT_COMMAND
128d113
< HCI_READ_EXTENDED_INQUIRY_RESPONSE_COMMAND
130d114
< HCI_READ_HOLD_MODE_ACTIVITY_COMMAND
135d118
< HCI_READ_LE_HOST_SUPPORT_COMMAND
137,138d119
< HCI_READ_LINK_SUPERVISION_TIMEOUT_COMMAND
< HCI_READ_LMP_HANDLE_COMMAND
156,157d136
< HCI_READ_SECURE_CONNECTIONS_HOST_SUPPORT_COMMAND
< HCI_READ_SIMPLE_PAIRING_MODE_COMMAND
172d150
< HCI_SET_AFH_HOST_CHANNEL_CLASSIFICATION_COMMAND
174d151
< HCI_SET_CONTROLLER_TO_HOST_FLOW_CONTROL_COMMAND
178c155
< HCI_SETUP_SYNCHRONOUS_CONNECTION_COMMAND
---
> HCI_SET_MIN_ENCRYPTION_KEY_SIZE_COMMAND
186,187d162
< HCI_WRITE_AFH_CHANNEL_ASSESSMENT_MODE_COMMAND
< HCI_WRITE_AUTHENTICATED_PAYLOAD_TIMEOUT_COMMAND
189d163
< HCI_WRITE_AUTOMATIC_FLUSH_TIMEOUT_COMMAND
195d168
< HCI_WRITE_HOLD_MODE_ACTIVITY_COMMAND
199d171
< HCI_WRITE_INQUIRY_TRANSMIT_POWER_LEVEL_COMMAND
210,211d181
< HCI_WRITE_SECURE_CONNECTIONS_TEST_MODE_COMMAND
< HCI_WRITE_SIMPLE_PAIRING_DEBUG_MODE_COMMAND |
|
Now I can connect Zephyr BAP server, as well as simpler devices like heart rate sensor (also running on native-sim) to my host computer and audio devices work with PipeWire now! |
In many cases you want your devices to find connection strong enough and not avoid connecting
|
Thank you for the update ! To clarify the situation with supported HCI commands: |
| }; | ||
|
|
||
| std::unique_ptr<PhyLayer> TestModel::CreatePhyLayer(PhyLayer::Identifier id, Phy::Type type, Phy::Model model) { | ||
| if (model == Phy::Model::IDEAL) { |
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.
Could you use a switch case preferably ?
| OpCodeIndex::READ_REMOTE_SUPPORTED_FEATURES, OpCodeIndex::READ_REMOTE_EXTENDED_FEATURES, | ||
| OpCodeIndex::READ_REMOTE_VERSION_INFORMATION, OpCodeIndex::READ_CLOCK_OFFSET, | ||
| OpCodeIndex::READ_LMP_HANDLE, OpCodeIndex::SETUP_SYNCHRONOUS_CONNECTION, | ||
| // OpCodeIndex::READ_LMP_HANDLE, OpCodeIndex::SETUP_SYNCHRONOUS_CONNECTION, |
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.
Careful, SETUP_SYNCHRONOUS_CONNECTION is implemented.
| BR_EDR, | ||
| }; | ||
|
|
||
| enum class Model { |
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.
PathLossModel ?
Uh oh!
There was an error while loading. Please reload this page.