nrf528xx immediatelly stop scan has sideeffects

[gen2thomas]

I have implemented a BLE-server with nrf52840 (which was switched to release v0.14.0 some days ago without any problem) and a client with nrf5280.

After switching from release v0.13.0 to v0.14.0 in the client, the scan function returns an error "invalid state, operation disallowed in this state" immediately when the target device (service) was detected. This is caused by the changes in #398 . After revert this one change in the client, it works without an error.

My callback-function looks like this:

callback := func(adapter *bluetooth.Adapter, result bluetooth.ScanResult) {
	// result.ManufacturerData() // sometimes causes "index out of range"
	if scanTimeout > 0 {
		timeOutElapsed = time.Since(startTime) > scanTimeout
	}

	if timeOutElapsed || result.Address.String() == identifier || result.LocalName() == identifier {
		// if the scan is stopped and no connection was done before (no initial connection), the nrf-hardware
		// returns "invalid state, operation disallowed in this state" for all later scans - so a stop-scan on a
		// timeout leads to a not repairable state, at least on the initial scan
		if e := adapter.StopScan(); e != nil {
			println("stop scan returns an error:", e.Error())
		}
		if !timeOutElapsed {
			finalResult = result
		}
	}
}

My comment in the code for the "timeout" is maybe the exact same reason, like described in #398. I will name it the "originated problem" now. Therefor I have set my "scanTimeout" to "0" to skip this code. Additionally I do not perform a "reconnect" but wait endless until the first connection is done successfully. This was my workaround for the originated problem.

The error is returned by the second "C.sd_ble_gap_scan_start()" call in the loop at line 74. So my assumption is: calling the function "C.sd_ble_gap_scan_stop()" when the function "C.sd_ble_gap_scan_start()" is still active, would lead to this behavior.

I wonder if the problem can be reproduced by someone else (@deadprogram , @zopieux)?

Maybe the adjustment with #398 needs to be improved, when other than that, the originated problem is fixed (I have not tested it yet by my own - I will remove my workaround in the next step).

[gen2thomas]

After some tests my previous assumption seem to be correct, because the callback is always used to stop the scan loop. This means "C.sd_ble_gap_scan_stop()" must not followed by "C.sd_ble_gap_scan_start(nil, &scanReportBufferInfo)", otherwise the latter returns the error "invalid state, operation disallowed in this state".

There are some possibilities to fix it.
V1: with existing code, return before calling "C.sd_ble_gap_scan_start(nil, &scanReportBufferInfo)", e.g. like this:

// Wait for received scan reports.
for a.scanning {
	// Wait for the next advertisement packet to arrive.
	// TODO: use some sort of condition variable once the scheduler supports
	// them.
	arm.Asm("wfe")
	if gotScanReport.Get() == 0 {
		// Spurious event. Continue waiting.
		continue
	}
	gotScanReport.Set(0)

	// Call the callback with the scan result.
	callback(a, globalScanResult)

	// break the loop if the callback has already stopped the scan, because start scan in this state is disallowed
	if a.scanning {
		break
	}
      
	// Restart the advertisement. This is needed, because advertisements are
	// automatically stopped when the first packet arrives.
	errCode := C.sd_ble_gap_scan_start(nil, &scanReportBufferInfo)
	if errCode != 0 {
		return Error(errCode)
	}
}

V2: revert #398, but call the stop-function after the loop

// Wait for received scan reports.
for a.scanning {
	// Wait for the next advertisement packet to arrive.
	// TODO: use some sort of condition variable once the scheduler supports
	// them.
	arm.Asm("wfe")
	if gotScanReport.Get() == 0 {
		// Spurious event. Continue waiting.
		continue
	}
	gotScanReport.Set(0)

	// Call the callback with the scan result.
	callback(a, globalScanResult)

	// Restart the advertisement. This is needed, because advertisements are
	// automatically stopped when the first packet arrives.
	errCode := C.sd_ble_gap_scan_start(nil, &scanReportBufferInfo)
	if errCode != 0 {
		return Error(errCode)
	}
}

// stop the device needs to be done, otherwise the next scan would fail
errCode = C.sd_ble_gap_scan_stop()
return makeError(errCode)

IMO, the first version V1 is more clean regarding an error on calling "C.sd_ble_gap_scan_stop()", which is returned from a stop-function and not from a scan-function.

Other optimizations/ideas/possibilities:

• in addition to "V1", change for a.scanning { to for {
• when using V1: if in addition to the "callback()" another go-routine is calling "StopScan()", e.g. responsible for watching the timeout, and this happens short before calling the "callback()", there needs to be an additional "if-not-scanning-break", otherwise 2 calls of "StopScan()" will exist, which would lead to an error "bluetooth: there is no scan in progress"
• same as before, but "inside" the callback - IMO this leads to unpredictable problems -> V2 is safer
• V3: Re-ordering the 3 blocks in the for-loop, so the callback is always after "C.sd_ble_gap_scan_start(nil, &scanReportBufferInfo)" - IMO the other problems described for "V1" still exists -> V2 is safer

[gen2thomas]

@deadprogram , @zopieux I have created an PR after some investigations and tests to fix/improve #398 .