fix(darwin): WriteWithoutResponse checks CanSendWriteWithoutResponse before request

[acouvreur]

bluetooth/gattc_darwin.go

Lines 200 to 208 in 5c61529

	// WriteWithoutResponse replaces the characteristic value with a new value. The
	// call will return before all data has been written. A limited number of such
	// writes can be in flight at any given time. This call is also known as a
	// "write command" (as opposed to a write request).
	func (c DeviceCharacteristic) WriteWithoutResponse(p []byte) (n int, err error) {
	c.service.device.prph.WriteCharacteristic(p, c.characteristic, false)
	return len(p), nil
	}

A limited number of such writes can be in flight at any given time.

When the underlying CoreBluetooth queue is full, a call to c.service.device.prph.WriteCharacteristic(p, c.characteristic, false) will silently fail.

The request is dropped.

CoreBluetooth provides canSendWriteWithoutResponse, which is available under the cbgo bindings as c.service.device.prph.CanSendWriteWithoutResponse().

---

This pull request adds a public function to the DeviceCharacteristic as CanSendWriteWithoutResponse().

This pull request also uses c.service.device.prph.CanSendWriteWithoutResponse() in WriteWithoutResponse() to properly return an error if the queue it cannot send a request at that time. Letting the user retry on the new error ErrCannotSendWriteWithoutResponse.

---

Example usage:

for i = 0; i < total; i+=chunkSize {
	end := i + chunkSize
	if end > total {
		end = total
	}

	for _, err := c.rx.WriteWithoutResponse(buffer) {
		if errors.Is(err, bluetooth.ErrCannotSendWriteWithoutResponse) {
			// Wait and retry
			<-time.After(5 * time.Millisecond)
			continue
		}
		if err != nil {
			return fmt.Errorf("failed to write to RX characteristic: %w", err)
		}
		break
	}

	fmt.Printf("Sent %d/%d bytes\r", end, len(buf))
}

[HattoriHanzo031]

I think you should rebase on dev, not release
https://github.com/tinygo-org/bluetooth/blob/release/CONTRIBUTING.md

[acouvreur]

I think you should rebase on dev, not release https://github.com/tinygo-org/bluetooth/blob/release/CONTRIBUTING.md

Done! Actually, dev and release are both up to date at that time

[gen2thomas]

Checking by another way than changing or writing is normally a bad practice. One reason against this is that there is a low probability that the verification will be OK, but the conditions may have changed by the time the actual call begins.

In best case the called method returns an error of type "BufferFull" or whatever causing the problem and the caller can react on that, e.g. by wait and retry.

Unfortunately in our case the called method "cbgo.Pheripheral.WriteCharacteristic" has no error. So, IMO, we should fix that problem at a higher level (with your PR). I assume that is what your proposal already means:

(maybe the WriteWithoutResponse command should call CanSendWriteWithoutResponse instead and fail if it returns false ?)

[acouvreur]

Checking by another way than changing or writing is normally a bad practice. One reason against this is that there is a low probability that the verification will be OK, but the conditions may have changed by the time the actual call begins.

In best case the called method returns an error of type "BufferFull" or whatever causing the problem and the caller can react on that, e.g. by wait and retry.

Unfortunately in our case the called method "cbgo.Pheripheral.WriteCharacteristic" has no error. So, IMO, we should fix that problem at a higher level (with your PR). I assume that is what your proposal already means:

(maybe the WriteWithoutResponse command should call CanSendWriteWithoutResponse instead and fail if it returns false ?)

That makes perfect sense, however CoreBluetooth itself is being used that way.

I'm not sure if it is possible to guarantee atomicity up to the driver level.

[gen2thomas]

I'm not sure if it is possible to guarantee atomicity up to the driver level.

Yes, this is not 100% possible at our side, but:

• the closer both calls are to each other, the better the requirements will be met
• the user of the existing function must not care about a second function, just implement its check and retry loop, if needed

But what I really not know: what kind of problems can lead to the state "CanSendWriteWithoutResponse()==false".
So our new error can not very explicit about that, e.g. something like var ErrSendImpossible = errors.New("send without response not possible") must be good enough.

[acouvreur]

I have updated my pull request to check if we can actually send the writewithoutresponse request, otherwise returning an error.

I've left the other function but we could remove it.

I think that is already far better than having the call to silently fail and drop the request withtout error.

---

But what I really not know: what kind of problems can lead to the state "CanSendWriteWithoutResponse()==false".

The only reason I think is that the internal queue of CoreBluetooth is full

Tip

canSendWriteWithoutResponse¹

If this value is false, flushing all current writes sets the value to true. This also results in a call to the delegate’s peripheralIsReady(toSendWriteWithoutResponse:).

Footnotes

1. https://developer.apple.com/documentation/corebluetooth/cbperipheral/cansendwritewithoutresponse ↩

[gen2thomas]

OK, looks good so far, please can you update your example, to show how to use it. A small remark: "n" and "err" can be removed now in line 207. A question: Do we really need the new mutex?

[acouvreur]

OK, looks good so far, please can you update your example, to show how to use it. A small remark: "n" and "err" can be removed now in line 207.

A question: Do we really need the new mutex?

How would you handle thread safe usage of this call without the mutex ?

We need to check and then send the message right ?

If we do not have the mutex, you do not have atomicity over the call.

[gen2thomas]

If we do not have the mutex, you do not have atomicity over the call.

I think this is true for all functions on receiver "DeviceCharacteristic" or at least for Write/Read related calls would it be needed. So making it thread safe is another story and because nobody has done this yet, also not for the other implementations (hci, linux, windows), I suppose it is planned to do it at caller side, if really needed.

We need to check and then send the message right ?

Yes, but if the caller do it not asynchronous (e.g. by using multiple go routines), there should be no problem, because no parallel calls can happen.

[acouvreur]

If we do not have the mutex, you do not have atomicity over the call.

I think this is true for all functions on receiver "DeviceCharacteristic" or at least for Write/Read related calls would it be needed. So making it thread safe is another story and because nobody has done this yet, also not for the other implementations (hci, linux, windows), I suppose it is planned to do it at caller side, if really needed.

We need to check and then send the message right ?

Yes, but if the caller do it not asynchronous (e.g. by using multiple go routines), there should be no problem, because no parallel calls can happen.

I agree, I can remove the mutex for now and open another issue about thread safe

[acouvreur]

@gen2thomas I have updated the example, although I do not like it. Maybe I should use some retry library to make it more clear ?

[acouvreur]

I have updated the example again with something slightly more good looking !