-
Notifications
You must be signed in to change notification settings - Fork 110
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
Write long characteristic #83
base: master
Are you sure you want to change the base?
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.
Thanks for the PR! I have a few suggestions as minor edits.
@@ -256,6 +256,11 @@ func (cln *Client) WriteCharacteristic(c *ble.Characteristic, b []byte, noRsp bo | |||
return nil | |||
} | |||
|
|||
// WriteLongCharacteristic writes a characteristic value that is longer than the MTU to a server. [Vol 3, Part F, 3.4.6] | |||
func (cln *Client) WriteLongCharacteristic(c *ble.Characteristic, b []byte) error { | |||
panic("not implemented") |
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.
Rather than panic, this should return an error. Forcing a panic upon a calling program is bit jarring for the caller to deal with.
prepareWriteLength := 0 | ||
|
||
for offset < len(v) { | ||
prepareWriteLength = MinInt( len(v) - offset, p.conn.TxMTU()-5 ) |
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.
Adding a function here adds a bit of complexity when an if
would suffice. Since MinInt
isn't used anywhere else, I don't see that it adds much value at this time. Additionally, I'd have to go review the spec to see recall why 5 is subtracted from the MTU here. I suspect it is overhead related. Please add a const which clarifies the reason for the subtraction.
Suggestion:
maxLength := p.conn.TxMTU() - 5
for offset < len(v) {
length := len(v) - offset
if length > maxLength {
length = maxLength
}
value := v[offset:offset+length]
...
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.
You could also avoid having to track offset
just by advancing v
:
maxLength := p.conn.TxMTU() - 5
for len(v) > 0 {
length := len(v)
if length > maxLength {
length = maxLength
}
...
v = v[length:]
}
@@ -401,3 +421,10 @@ type sub struct { | |||
nHandler ble.NotificationHandler | |||
iHandler ble.NotificationHandler | |||
} | |||
|
|||
func MinInt( a int, b int) int { |
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.
If you are ok with my suggestion above, this function can be removed.
WriteLongCharacteristic was not implemented. Here is it implemented for linux, but I have not implemented it for darwin
This also fixes a but in PrepareWrite(), where the value had not been written.
This code was tested against a raspberry pi and a custom bluetooth device