Conversation
estutzenberger
left a comment
estutzenberger
left a comment
There was a problem hiding this comment.
Thanks for the PR! I have a few suggestions as minor edits.
|
|
||
| // 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.
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.
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.
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:]
}| iHandler ble.NotificationHandler | ||
| } | ||
|
|
||
| func MinInt( a int, b int) int { |
There was a problem hiding this comment.
If you are ok with my suggestion above, this function can be removed.
3 participants
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