SweetBlue icon indicating copy to clipboard operation
SweetBlue copied to clipboard

Writting long byte returns with error on FOTA update

Open AlejandroHCruz opened this issue 6 years ago • 13 comments

Dear SweetBlue Team,

We've been experimenting with your library, considering the idea of fully migrating our Bluetooth communication to it with a commercial license. So far we've been able to connect our custom hardware peripheral, setup notifications and send/receive writing operations. We're really happy with the results so far on the general usage of your software.

We're now in the final test, which consists in building a successful FOTA (Firmware-Over-The-Air, as you know) update with SweetBlue. I hope you can help us or point us in the right direction, all tips and insights are welcomed!

We're using a PSoC from Cypress, which doesn't make the FOTA particularly simple, this needs to be written with a response:

  1. Enter Bootloader.
  2. Send Data*.
  3. Write Data*.
  4. Confirm Data was written successfully*.
  • = happens multiple times.

Since it's a complex OTA operation and building a custom BleTransaction.Ota has not been successful for performing all of the steps, we've stayed true to the Cypress implementation of request-response, just migrating the write and callback methods to SweetBlue.

Replacing the mBluetoothGatt.writeCharacteristic(characteristic) function and BluetoothGattCallback subclass with deviceToUpgrade.write(characteristic.getService.getUuid(), characteristic.getUuid(), valueByte, readWriteEvent -> { ... }

Specifically, we're having issues on the second step, in which we would send a byte[] array like this one:

01 39 83 00 00 3B 00 00 40 00 20 91 1D 00 00 51 88 00 00 51 88 00 00 10 B5 02 4B 83 F3 08 88 06 F0 7A FD 00 40 00 20 F7 B5 72 B6 00 24 01 22 92 4B 93 4D 21 00 2C 60 1A 70 E0 22 91 48 92 00 0A F0 8E FC 80 22 21 00 D2 00 8F 48 0A F0 88 FC 80 22 8E 49 8E 48 0A F0 7A FC 00 95 21 00 22 00 FF 25 8C 4E 28 36 8B 4B C8 58 53 00 07 00 28 40 12 18 50 00 01 30 01 33 30 18 AF 43 F3 18 01 90 01 98 98 42 07 D0 58 1E 59 D5 17

We write the operation successfully but the PSoC returns the following error, which is defined by Cypress as:

BOOTLOADER_ERR_DATA (0x04) /* The data is not of the proper form */ which probably comes from #define Bootloader_COMMAND_DATA (0x37u) /* Queue up a block of data for programming */

-> Could it be that we shouldn't be using the write function but rather another one such as performTransaction for each line/byte[] send/write/verify?

-> Our app is paired with the peripheral through the device.bond() method, does that account as the required bonding state?

-> What else could be the issue here? The byte[], characteristic and service are exactly the same on both implementations.


This is the building method for the byte[], it shouldn't be problematic but I place it here as a reference.

`
/** * OTA Bootloader Program row Command * */ public void OTAProgramRowCmd(long rowMSB, long rowLSB, int arrayID, byte[] data, String checkSumType) {

    int COMMAND_DATA_SIZE = 3;
    int totalSize = BootLoaderCommands.BASE_CMD_SIZE + COMMAND_DATA_SIZE +
            data.length;
    int checksum;
    int i;
    byte[] commandBytes = new byte[totalSize];
    int startCommand = 0x01;

    commandBytes[BYTE_START_CMD] = (byte) startCommand;
    commandBytes[BYTE_CMD_TYPE] = (byte) BootLoaderCommands.PROGRAM_ROW;
    commandBytes[BYTE_CMD_DATA_SIZE] = (byte) (data.length + COMMAND_DATA_SIZE);
    commandBytes[BYTE_CMD_DATA_SIZE_SHIFT] = (byte) ((int) ((data.length + COMMAND_DATA_SIZE) >> ADDITIVE_OP));
    commandBytes[BYTE_ARRAY_ID] = (byte) arrayID;
    commandBytes[BYTE_ROW] = (byte) rowMSB;
    commandBytes[6] = (byte) rowLSB;
    for (i = 0; i < data.length; i++)
        commandBytes[i + 7] = data[i];
    checksum = BootLoaderUtils.calculateCheckSum2(Integer.parseInt(checkSumType, RADIX),
            data.length + 7, commandBytes);
    commandBytes[totalSize - 3] = (byte) checksum;
    commandBytes[totalSize - 2] = (byte) (checksum >> ADDITIVE_OP);
    commandBytes[totalSize - 1] = (byte) BootLoaderCommands.PACKET_END;

    FOTAManager.writeOTABootLoaderCommand(mOTACharacteristic, commandBytes);

}`

TL;DR: Is there a length limit or set of best practices for sending long bytes with the multiple methods? Is SweetBlue breaking down or wrapping up long byte arrays in the write function and we should rather be using performTransaction?

AlejandroHCruz avatar Mar 27 '18 14:03 AlejandroHCruz

Ok, in BLE, the default MTU (maximum transmission unit) is 23 bytes (with an effective 20 byte payload...3 bytes for overhead). SweetBlue does have a feature called write striping, however that is going to be deprecated, as it ends up creating more problems than it solves. If you try to send a write bigger than the current MTU, it will create a new transaction, and split up the byte array into separate writes. This works fine normally, but if you try it within another transaction, it won't work (you can't run a transaction within another transaction). SO, my suggestion would be to split up your byte array so that it sends only in 20 byte increments (manually). Hopefully the firmware supports this. The other alternative would be to request a larger MTU size. This also depends on the firmware supporting it (also note that phones running android < 5.0 can't request MTU changes). BleDevice.setMtu() is the method you want to use. Also note that while some phone allow you to try to set a larger MTU size, and will tell you it worked, when you try to write with a payload of the new mtu size, it will time out (Take a look at BleDeviceConfig.MtuTestCallback).

BTW, you list in your steps "Send Data", then "Write data". What's the difference? To send data is to "write" to the device, unless I'm misunderstanding something.

ryanhubbell avatar Mar 27 '18 15:03 ryanhubbell

Thank you for your feedback, it's really valuable. I'm currently looking into the option to change the default MTU and running some tests.

I'd like to also reference this comment on another forum, could you tell me how to specify the write type on SweetBlue or would changing the MTU internally translate into that?

According to this commit of the Android-nRF-Toolbox project, long writes are supported by the Android BLE stack if the write type is set to WRITE_TYPE_DEFAULT (ATT write request).

https://github.com/NordicSemiconductor/Android-nRF-Toolbox/commit/341ecf5781f9e8ba5b979d5b6c94a2683bdfb304#commitcomment-17511946

AlejandroHCruz avatar Mar 28 '18 09:03 AlejandroHCruz

I'm not sure how much stock I'd put into that. It's certainly possible, but the fact the guy said "Tested, works on all phones", but when asked if you get one callback, or 5, he had no idea. So how do you know if it worked without checking the callback? Seems suspect to me. WRITE_TYPE_DEFAULT is well, the default. No need to set it to that because it already is the default, hence the name. The other options are WRITE_TYPE_NO_RESPONSE, and WRITE_TYPE_SIGNED. If you absolutely want to set it, then use the WriteBuilder class can call setWriteType(). ReadWriteListener.Type.WRITE is what equates to WRITE_TYPE_DEFAULT.

What they're saying in that forum is that android under the hood supposedly will split up the byte array into 20 byte chunks for you (this is not something I would feel comfortable with...anytime android tries to do something automatically for you, it usually messes something up). It's easy enough to split up a byte[] yourself. Utils_Byte.subBytes will allow you to pull out chunks easily.

Setting the MTU will allow you to transmit the whole byte array in one packet (as opposed to splitting it up into smaller packets to be sent individually). Not only is this easier, and less error prone, but it will be faster because there is only one packet going over the air.

ryanhubbell avatar Mar 28 '18 12:03 ryanhubbell

Thanks again, let me set the type to keep the code as close as how it was before changing to SweetBlue for now. We were also explicitly stating the type to be the default one for some reason.

On the firmware side, the maximum package size is 300 so it should be supported. I'm still unsure about it working with the splitting, it's not working so far, will keep trying all the options that you've mentioned here, the Utils_Byte looks really useful.

Also, it makes sense for the OS to be splitting the packages by itself and to fail in the process, thus producing the errors on the Firmware Update that we've experienced in the past on certain phones, for apparently no reason.

When setting the MTU, is checking the wasSuccess() of the callback that's passed as a parameter enough confirmation that it worked? Doesn't seem to be getting set on an Android 6.0.1 device even though setting the MTU succeeded... and the fact that the function returns another ReadWriteEvent makes it a bit confusing. The returned ReadWriteEvent is to be ignored, correct?

BTW: These are the official differences between sending and writing the data

Bootloader_COMMAND_DATA -> Queue up a block of data for programming Bootloader_COMMAND_PROGRAM -> Program the specified row
Bootloader_COMMAND_VERIFY -> Compute flash row checksum for verification

AlejandroHCruz avatar Mar 28 '18 14:03 AlejandroHCruz

The ReadWriteEvent instance that gets returned from the method call is so you can see if the operation early outed or not. It can be ignored, as even in this case, the callback will be called (however, you're not forced to provide a listener). With regards to the MTU succeeding, how do you know it's not being set? Also, have you looked at MtuTestCallback like I mentioned in my first reply? We've noticed there are at least 2 devices we know of that report that they set the MTU, but when you try to write a payload the size of the MTU, it fails. This is the purpose of the MtuTestCallback...basically, you need to provide the service and characteristic Uuid that you can write a large amount of data to.

ryanhubbell avatar Mar 28 '18 17:03 ryanhubbell

Feel like I'm getting to understand everything.

I'll look into the MtuTestCallback as you suggested, what I'm doing so far is, once the device is in the BleDeviceState.INITIALIZED is:

                 if (deviceToUpgrade.getMtu() < DESIRED_LENGTH ) {

                    deviceToUpgrade.setMtu(DESIRED_LENGTH, readWriteListener -> {
                        if (readWriteListener.wasSuccess()) Log.d(TAG, "setMtu SUCCESS: " + DESIRED_LENGTH + ", " + deviceToUpgrade.getMtu());
                        else Log.e(TAG, setMtu ERROR: " + deviceToUpgrade.getMtu());
                    });

Here, even when it was successful, the deviceToUpgrade.getMtu() returns 23 and I can also see the native logging as follows:

D/BluetoothGatt: onConfigureMTU() - Device=5F:0E:DA:AD:93:1F mtu=23 status=0

I though it could be a weird timing issue so also checked again the mtu before writing the operation, but it still shows 23.

AlejandroHCruz avatar Mar 28 '18 17:03 AlejandroHCruz

I think I know what's up. I'm thinking I need to refactor the name of that method to requestMtu. When trying to change the MTU, it is a negotiation with the device. So really, the success part is the fact that the android device and peripheral successfully negotiated the MTU. The ReadWriteEvent holds the negotiated mtu size, so you can check that when the callback is invoked.

SO, it looks like the firmware doesn't support changing the MTU size. How much control do you have over the firmware?

ryanhubbell avatar Mar 28 '18 17:03 ryanhubbell

I see, yeah requestMtu makes more sense for an asynchronous request. We have full control of the Firmware but it should work as it is right now, because otherwise it's a circular problem in which you need a firmware update to get the firmware update to work...

I know for a fact that the MTU on our firmware is 300u so changing it to support larger byte arrays should not even be done? If this is true, should the Android OS or SweetBlue still get notified to allow such large bytes to be sent out?

AlejandroHCruz avatar Mar 28 '18 20:03 AlejandroHCruz

Hi, my conclusion is that the SweetBlue library is doing something wrong. Further ideas/assistance is welcomed.

This works:

        if (valueByte.length > 20) {

            characteristic.setValue(valueByte);
            characteristic.setWriteType(BluetoothGattCharacteristic.WRITE_TYPE_DEFAULT);
            boolean status = deviceToUpgrade.getNativeGatt().writeCharacteristic(characteristic);

            Log.d(TAG, "Old style write executed with status: " + status);
         }

This doesn't:

        if (valueByte.length > 20) {

            writeBuilder
                    .setBytes(valueByte)
                    .setWriteType(BleDevice.ReadWriteListener.Type.WRITE) // WRITE_TYPE_DEFAULT
                    .setCharacteristicUUID(UUIDDatabase.UUID_OTA_CHARACTERISTIC)
                    .setServiceUUID(UUIDDatabase.UUID_OTA_UPDATE_SERVICE);

            deviceToUpgrade.write(writeBuilder);
         }

AlejandroHCruz avatar Apr 03 '18 16:04 AlejandroHCruz

Could you be more specific than "this doesn't work"?

This is what the library does: Write Typing:

if (m_writeType != null)
        {
            if (m_writeType == BleDevice.ReadWriteListener.Type.WRITE_NO_RESPONSE)
            {
                char_native.setWriteType(BluetoothGattCharacteristic.WRITE_TYPE_NO_RESPONSE);
            }
            else if (m_writeType == BleDevice.ReadWriteListener.Type.WRITE_SIGNED)
            {
                char_native.setWriteType(BluetoothGattCharacteristic.WRITE_TYPE_SIGNED);
            }
            else if (char_native.getWriteType() != BluetoothGattCharacteristic.WRITE_TYPE_DEFAULT)
            {
                char_native.setWriteType(BluetoothGattCharacteristic.WRITE_TYPE_DEFAULT);
            }
        }

Then to write:

if( false == getDevice().layerManager().setCharValue(char_native, m_data) )
        {
            fail(BleDevice.ReadWriteListener.Status.FAILED_TO_SET_VALUE_ON_TARGET, BleStatuses.GATT_STATUS_NOT_APPLICABLE, getDefaultTarget(), getCharUuid(), BleDevice.ReadWriteListener.ReadWriteEvent.NON_APPLICABLE_UUID);
        }
        else
        {
            if( false == getDevice().layerManager().writeCharacteristic(char_native) )
            {
                fail(BleDevice.ReadWriteListener.Status.FAILED_TO_SEND_OUT, BleStatuses.GATT_STATUS_NOT_APPLICABLE, getDefaultTarget(), getCharUuid(), BleDevice.ReadWriteListener.ReadWriteEvent.NON_APPLICABLE_UUID);
            }
            else
            {
                // SUCCESS, for now...
            }
        }

setCharValue is just calling characteristic.setValue. So as you can see, it's doing the same thing, only with more error checking.

ryanhubbell avatar Apr 04 '18 15:04 ryanhubbell

Sorry if my language was not clear:

I meant that the paired device is returning the aforementioned error (data being in a wrong format).

I assume, based on the commit mentioned before that the WRITE_TYPE_DEFAULT needs to be explicitly set, even if the type is already the default one, for the Android OS to actually send out long bytes otherwise it's still chopping them to meet the regular MTU limit.

AlejandroHCruz avatar Apr 05 '18 16:04 AlejandroHCruz

If you look at the source code for setWriteType,

/**
     * Set the write type for this characteristic
     *
     * <p>Setting the write type of a characteristic determines how the
     * {@link BluetoothGatt#writeCharacteristic} function write this
     * characteristic.
     *
     * @param writeType The write type to for this characteristic. Can be one
     *                  of:
     *                  {@link #WRITE_TYPE_DEFAULT},
     *                  {@link #WRITE_TYPE_NO_RESPONSE} or
     *                  {@link #WRITE_TYPE_SIGNED}.
     */
    public void setWriteType(int writeType) {
        mWriteType = writeType;
    }

I don't see how that would do anything different. I could see if there was some extra logic in the set method, but there isn't.

ryanhubbell avatar Apr 05 '18 16:04 ryanhubbell

I see your point. Then I'm out of ideas regarding that specific issue.

AlejandroHCruz avatar Apr 05 '18 16:04 AlejandroHCruz