Linux implementation of WriteEvent callback does not correctly update the characteristic's value for subsequent reading

[MykolaRodin]

Hi,

Prerequisites:

My OS is Ubuntu 20.04 with default Bluez 5.53. My laptop is HP with Realtek Bluetooth chip. It looks like I have encountered an issue when trying to implement WriteEvent callback for a characteristic.

Root cause:

In accordance with https://github.com/tinygo-org/bluetooth/blob/release/examples/ledcolor/main.go it can be performed like that

            must("add service", adapter.AddService(&bluetooth.Service{
		UUID: bluetooth.NewUUID(serviceUUID),
		Characteristics: []bluetooth.CharacteristicConfig{
			{
				Handle: &ledColorCharacteristic,
				UUID:   bluetooth.NewUUID(charUUID),
				Value:  ledColor[:],
				Flags:  bluetooth.CharacteristicReadPermission | bluetooth.CharacteristicWritePermission,
				WriteEvent: func(client bluetooth.Connection, offset int, value []byte) {
					if offset != 0 || len(value) != 3 {
						return
					}
					ledColor[0] = value[0]
					ledColor[1] = value[1]
					ledColor[2] = value[2]
					hasColorChange = true
				},
			},
		},
	}))

Here the initial characteristic value is set to ledColor[:] and on WriteEvent ledColor is updated by a new value. In https://github.com/tinygo-org/bluetooth/blob/release/gatts_linux.go the callback is registered like that:

                // Do a callback when the value changes.
		if char.WriteEvent != nil {
			callback := char.WriteEvent
			bluezChar.OnWrite(func(c *service.Char, value []byte) ([]byte, error) {
				// BlueZ doesn't seem to tell who did the write, so pass 0
				// always.
				// It also doesn't provide which part of the value was written,
				// so pretend the entire characteristic was updated (which might
				// not be the case).
				callback(0, 0, value)
				return nil, nil
			})
		}

On the other hand, muka go-bluetooth implementation for Linux uses this way for handling the callbacks under the hood in https://github.com/muka/go-bluetooth/blob/master/api/service/app_char_rw.go:

    func (s *Char) ReadValue(options map[string]interface{}) ([]byte, *dbus.Error) {

	log.Debug("Characteristic.ReadValue")
	if s.readCallback != nil {
		b, err := s.readCallback(s, options)
		if err != nil {
			return nil, dbus.MakeFailedError(err)
		}
		return b, nil
	}

	return s.Properties.Value, nil
    }
...
    func (s *Char) WriteValue(value []byte, options map[string]interface{}) *dbus.Error {

	log.Trace("Characteristic.WriteValue")

	val := value
	if s.writeCallback != nil {
		log.Trace("Used write callback")
		b, err := s.writeCallback(s, value)
		val = b
		if err != nil {
			return dbus.MakeFailedError(err)
		}
	} else {
		log.Trace("Store directly to value (no callback)")
	}

	// TODO update on Properties interface
	s.Properties.Value = val
	err := s.iprops.Instance().Set(s.Interface(), "Value", dbus.MakeVariant(value))

	return err
    }

If I am getting it right, in accordance with this implementation the characteristic value is updated on writing by the value that the callback returns. Yet in https://github.com/tinygo-org/bluetooth/blob/release/gatts_linux.go the callback is called first like callback(0, 0, value) and then nil, nil is returned. As result, the characteristic value is updated by nil value at s.Properties.Value = val. Subsequent readings should return this nil value through return s.Properties.Value, nil.

Proposed fix:

If this behavior is not expected, it can be fixed by changing the callback type and returning the right value from the callback like

                // Do a callback when the value changes.
		if char.WriteEvent != nil {
			callback := char.WriteEvent
			bluezChar.OnWrite(func(c *service.Char, value []byte) ([]byte, error) {
				// BlueZ doesn't seem to tell who did the write, so pass 0
				// always.
				// It also doesn't provide which part of the value was written,
				// so pretend the entire characteristic was updated (which might
				// not be the case).
				
				return callback(0, 0, value), nil // Or `return callback(0, 0, value)` in case the callback returns both the value and the error
			})
		}

In this case the callback type should be changed from func(client bluetooth.Connection, offset int, value []byte) to func(client bluetooth. Connection, offset int, value []byte) []byte or func(client bluetooth.Connection, offset int, value []byte) ([]byte, error).

Please let me know if I am missing something.

[deadprogram]

Since the Linux interface was rewritten, is this still an issue?