- 
                Notifications
    
You must be signed in to change notification settings  - Fork 79
 
Typos and some other things... #476
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -34,13 +34,9 @@ debug = 2 | |
| 
     | 
||
| [patch.crates-io] | ||
| #embassy-executor = {path = "../../../embassy/embassy-executor"} | ||
| #embassy-time-queue-utils = {path = "../../../embassy/embassy-time-queue-utils"} | ||
| #embassy-sync = {path = "../../../embassy/embassy-sync"} | ||
| #embassy-futures = {path = "../../../embassy/embassy-futures"} | ||
| #embassy-time = {path = "../../../embassy/embassy-time"} | ||
| #embassy-time-driver = {path = "../../../embassy/embassy-time-driver"} | ||
| #embassy-embedded-hal = {path = "../../../embassy/embassy-embedded-hal"} | ||
| #embassy-hal-internal = {path = "../../../embassy/embassy-hal-internal"} | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, and below, I decided to prune the  If there's a comment, refering to a crate that's not in the   | 
||
| nrf-sdc = { git = "https://github.com/alexmoon/nrf-sdc.git", rev = "11d5c3c"} | ||
| nrf-mpsl = { git = "https://github.com/alexmoon/nrf-sdc.git", rev = "11d5c3c"} | ||
| #bt-hci = { git = "https://github.com/embassy-rs/bt-hci.git", rev = "83a9e179ff019ed60abb73705f54383f89fc60fc" } | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -39,16 +39,9 @@ debug = 2 | |
| 
     | 
||
| [patch.crates-io] | ||
| #embassy-executor = {path = "../../../embassy/embassy-executor"} | ||
| #embassy-nrf = {path = "../../../embassy/embassy-nrf"} | ||
| #embassy-sync = {path = "../../../embassy/embassy-sync"} | ||
| #embassy-futures = {path = "../../../embassy/embassy-futures"} | ||
| #embassy-time = {path = "../../../embassy/embassy-time"} | ||
| #embassy-time-driver = {path = "../../../embassy/embassy-time-driver"} | ||
| #embassy-embedded-hal = {path = "../../../embassy/embassy-embedded-hal"} | ||
| #embassy-hal-internal = {path = "../../../embassy/embassy-hal-internal"} | ||
| #nrf-sdc = { path = "../../../nrf-sdc/nrf-sdc" } | ||
| #nrf-mpsl = { path = "../../../nrf-sdc/nrf-mpsl" } | ||
| #bt-hci = { path = "../../../bt-hci" } | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. e.g.   | 
||
| cyw43 = { git = "https://github.com/embassy-rs/embassy.git", rev = "faacad613ea26a28de216cf32ac4cc64f7862d4c" } | ||
| embassy-time = { git = "https://github.com/embassy-rs/embassy.git", rev = "faacad613ea26a28de216cf32ac4cc64f7862d4c" } | ||
| embassy-time-driver = { git = "https://github.com/embassy-rs/embassy.git", rev = "faacad613ea26a28de216cf32ac4cc64f7862d4c" } | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -253,7 +253,7 @@ pub struct AttributeServer< | |
| > { | ||
| att_table: AttributeTable<'values, M, ATT_MAX>, | ||
| cccd_tables: CccdTables<M, CCCD_MAX, CONN_MAX>, | ||
| _p: PhantomData<P>, | ||
| _p: PhantomData<P>, // please comment on the purpose | ||
| } | ||
| 
     | 
||
| pub(crate) mod sealed { | ||
| 
          
            
          
           | 
    @@ -939,8 +939,9 @@ mod tests { | |
| while let Some(att) = it.next() { | ||
| let handle = att.handle; | ||
| let uuid = &att.uuid; | ||
| // bad format: "last_handle_in_group for 0x{:0>4x?}, 0x{:0>2x?} 0x{:0>2x?}", // "Unknown display hint: '0>4x?' | ||
| trace!( | ||
| "last_handle_in_group for 0x{:0>4x?}, 0x{:0>2x?} 0x{:0>2x?}", | ||
| "last_handle_in_group for 0x{:04x}, 0x{:02x} 0x{:02x}", // not sure which is the intended format, here! | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My IDE (Rust Rover) showed: 
 It seems that's indeed an invalid combination (asked Copilot). What is the intended output - what should be here? Also, shouldn't CI or something pick this up? I'm confused, please help. O:)  | 
||
| handle, | ||
| uuid, | ||
| att.last_handle_in_group | ||
| 
          
            
          
           | 
    @@ -981,7 +982,8 @@ mod tests { | |
| ) | ||
| .unwrap(); | ||
| let response = &buffer[0..length]; | ||
| trace!(" 0x{:0>2x?}", response); | ||
| // bad format: " 0x{:0>2x?}" <-- "unknown display hint" | ||
| trace!(" 0x{:02x}", response); | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ..here, too  | 
||
| // It should be a successful response, because the service should be found, this will assert if | ||
| // we failed to retrieve the third service. | ||
| assert_eq!(response[0], att::ATT_READ_BY_GROUP_TYPE_RSP); | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -474,14 +474,14 @@ impl<'d, P: PacketPool> ChannelManager<'d, P> { | |
| let req = ConnParamUpdateReq::from_hci_bytes_complete(data)?; | ||
| debug!("[l2cap][conn = {:?}] connection param update request: {:?}", conn, req); | ||
| let interval_min: bt_hci::param::Duration<1_250> = bt_hci::param::Duration::from_u16(req.interval_min); | ||
| let interva_max: bt_hci::param::Duration<1_250> = bt_hci::param::Duration::from_u16(req.interval_max); | ||
| let interval_max: bt_hci::param::Duration<1_250> = bt_hci::param::Duration::from_u16(req.interval_max); | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one's interesting. It's a typo in a variable name. Now.. 
 Kindly, it would also be great if unused variables were reported. Have I missed something - didn't get anything on the IDE or the CI output. What's really going on?  | 
||
| let timeout: bt_hci::param::Duration<10_000> = bt_hci::param::Duration::from_u16(req.timeout); | ||
| use embassy_time::Duration; | ||
| let _ = manager.post_handle_event( | ||
| conn, | ||
| ConnectionEvent::RequestConnectionParams { | ||
| min_connection_interval: Duration::from_micros(interval_min.as_micros()), | ||
| max_connection_interval: Duration::from_micros(interval_min.as_micros()), | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
  | 
||
| max_connection_interval: Duration::from_micros(interval_min.as_micros()), // Q: why didn't anything catch 'interva_max' unused? Isn't that worth fixing? 🤨 | ||
| max_latency: req.latency, | ||
| supervision_timeout: Duration::from_micros(timeout.as_micros()), | ||
| }, | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -717,7 +717,7 @@ impl<'d, C: Controller, P: PacketPool> Runner<'d, C, P> { | |
| + ControllerCmdSync<LeCreateConnCancel> | ||
| + ControllerCmdSync<LeSetScanEnable> | ||
| + ControllerCmdSync<LeSetExtScanEnable> | ||
| + for<'t> ControllerCmdSync<LeSetAdvEnable> | ||
| + ControllerCmdSync<LeSetAdvEnable> | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I belive, but am not 100% sure, that the   | 
||
| + for<'t> ControllerCmdSync<LeSetExtAdvEnable<'t>> | ||
| + for<'t> ControllerCmdSync<HostNumberOfCompletedPackets<'t>> | ||
| + ControllerCmdSync<LeReadBufferSize> | ||
| 
        
          
        
         | 
    @@ -741,7 +741,7 @@ impl<'d, C: Controller, P: PacketPool> Runner<'d, C, P> { | |
| + ControllerCmdSync<HostBufferSize> | ||
| + ControllerCmdAsync<LeConnUpdate> | ||
| + ControllerCmdSync<SetControllerToHostFlowControl> | ||
| + for<'t> ControllerCmdSync<LeSetAdvEnable> | ||
| + ControllerCmdSync<LeSetAdvEnable> | ||
| + for<'t> ControllerCmdSync<LeSetExtAdvEnable<'t>> | ||
| + for<'t> ControllerCmdSync<HostNumberOfCompletedPackets<'t>> | ||
| + ControllerCmdSync<LeSetScanEnable> | ||
| 
          
            
          
           | 
    @@ -1039,7 +1039,7 @@ impl<'d, C: Controller, P: PacketPool> ControlRunner<'d, C, P> { | |
| + ControllerCmdSync<SetControllerToHostFlowControl> | ||
| + ControllerCmdSync<Reset> | ||
| + ControllerCmdSync<LeCreateConnCancel> | ||
| + for<'t> ControllerCmdSync<LeSetAdvEnable> | ||
| + ControllerCmdSync<LeSetAdvEnable> | ||
| + for<'t> ControllerCmdSync<LeSetExtAdvEnable<'t>> | ||
| + ControllerCmdSync<LeSetScanEnable> | ||
| + ControllerCmdSync<LeSetExtScanEnable> | ||
| 
          
            
          
           | 
    ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -3,9 +3,6 @@ use embassy_time::Duration; | |
| /// 128-bit encryption key size | ||
| pub(crate) const ENCRYPTION_KEY_SIZE_128_BITS: u8 = 128 / 8; | ||
| 
     | 
||
| /// Long duration, to disable the timer | ||
| pub(crate) const TIMEOUT_DISABLE: Duration = Duration::from_secs(3600 * 24 * 365 * 10); // ~10 years | ||
| // Workaround for Duration multiplication not being const | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just cleanup. The const was used only as   | 
||
| const TIMEOUT_SECS: u64 = 30; | ||
| /// Pairing time-out | ||
| pub(crate) const TIMEOUT: Duration = Duration::from_secs(TIMEOUT_SECS); | ||
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.
Such comments were intended to be removed, before merging, in an earlier PR. Now's a good time.