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

Add missing macType param to NO_DEVICE_FOUND #1253

Merged
merged 2 commits into from
Aug 9, 2022

Conversation

kabili207
Copy link
Contributor

Description:

This addresses an issue that was created back when the mac_type was first added back in January with f8b4b3c, which was causing every BLE device to be marked as connectable when first detected. Combine this with BLE_FILTER_CONNECTABLE 1 and suddenly every device is getting filtered out before OMG has the chance to initialize anything.

Checklist:

  • The pull request is done against the latest development branch
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • I accept the DCO.

@kabili207
Copy link
Contributor Author

kabili207 commented Aug 9, 2022

I should point out that there is also a mismatch regarding macAddr as well: it's defined as char[18], but we're only initializing 12 values. It's not really a functional issue though, since the missing array values will still get initialized to 0 at compile time.

@h2zero
Copy link
Collaborator

h2zero commented Aug 9, 2022

Good catch!

I should point out that there is also a mismatch regarding macAddr as well: it's defined as char[18], but we're only initializing 12 values.

I don't agree here though, the mac (ID) when creating the new BLEDevice uses the string value of the mac returned, which contains 12 chars of the address + 5* ":" separators, and of course the null char at the end. The ":"s are then later removed when publishing, which is what you're seeing in the logs/mqtt messages.
Just wanted to clarify, certainly an area for optimization though. Again, good catch!

@kabili207
Copy link
Contributor Author

It was driving me up the wall trying to trace the issue since I wanted to enable that flag. These little quirks in C/C++ are always fun to find.

The mismatch I was referring to was in the NO_DEVICE_FOUND definition, where we're setting it to {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, which is only 12 zeroes instead of the expected 18. It's still technically valid since the missing 6 array values will get defaulted to 0, but it did stick out to me since I was already counting parameters in the same area.

@h2zero
Copy link
Collaborator

h2zero commented Aug 9, 2022

It was driving me up the wall trying to trace the issue since I wanted to enable that flag. These little quirks in C/C++ are always fun to find.

Keeps me entertained lol, things like this are a real pain to find though 👍

The mismatch I was referring to was in the NO_DEVICE_FOUND definition, where we're setting it to {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, which is only 12 zeroes instead of the expected 18.

Yes, you're correct, the whole thing could just be changed to {0} for this case. If you're inclined to do so you could add that to this PR.

@kabili207
Copy link
Contributor Author

Yes, you're correct, the whole thing could just be changed to {0} for this case. If you're inclined to do so you could add that to this PR.

Added! Better to fix it now rather than later.

@h2zero
Copy link
Collaborator

h2zero commented Aug 9, 2022

Perfect, thanks!

Copy link
Collaborator

@h2zero h2zero left a comment

Choose a reason for hiding this comment

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

👍

@1technophile
Copy link
Owner

Thanks!

@1technophile 1technophile merged commit b17247c into 1technophile:development Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants