-
Notifications
You must be signed in to change notification settings - Fork 138
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
enable setting MACAddr for cyw43439 #280
Conversation
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.
A couple of questions and suggestions
hciPacket[i] = address.MACAddress.MAC[len(address.MACAddress.MAC)-1-i] | ||
} | ||
|
||
if err := h.sendWithoutResponse(ogfVendor<<ogfCommandPos|ocfSetBTMACAddr, hciPacket); err != nil { |
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 documented somewhere? If this is a known HCI command then this should not be behind a cyw43439 build tag.
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.
Only works for this board. The opcode is vendor-specific
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.
Then this may belong in cyw43439 repo- given the hci packet header is a constant value and need not full hci logic to build. If the logic in this package is needed then this function may belong in an example directory under cyw43439 that uses this repo. In any case I'd avoid adding this exported API until its clearer it belongs here.
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.
#229 sets the MAC address using a standard opcode - does that work on CYW43439?
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 tried that first! didn't seem to work for me. I have two boards and neither worked with that PR, which is why I looked into the spec to see what was recommended, which is how I arrived at this
ocfSetBTMACAddr = 0x0001 | ||
) | ||
|
||
func (a *Adapter) SetBdAddr(address Address) error { |
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.
Not a fan of the naming- I'd prefer SetAddress
since the type it receives is an Address
... or even SetBluetoothAddress
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.
Sure I can rename it. The function to get the Bluetooth address was named GetBdAddr which is where the name comes from
Thanks @soypat I've responded to each. Let me know which should become changes |
Seems like all feedback has been addressed 🥲 Now merging, thanks @taigrr for the addition and all the reviewers for such great feedback! |
cyw43439 boards don't seem to consistently set a MAC address from a seeded value, and always use a hardcoded MACAddr of
43:43:A2:12:1F:AC
Therefore, in order to use two Picos near each other, it's necessary to provide a Set() call for changing the address.
This issue is highlighted by others as well below:
raspberrypi/pico-sdk#1323
I initially opened an issue here:
soypat/cyw43439#52
However, after implementing it I felt it made more sense to get the code into this repo with build tags.
This change has been tested and validated against my gopher2040-w badges from Gophercon 2024, which use a pico board.
This is my first PR into this repo, so feel free to hit me with style nits or have me change my function names/signatures or just tell me it should go into the other repo.