Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Terrible handling of initLen / minLen and variable length characteristics. #93

Closed
Timmmm opened this issue Oct 30, 2015 · 6 comments
Closed

Comments

@Timmmm
Copy link

Timmmm commented Oct 30, 2015

See: https://devzone.nordicsemi.com/question/34390/no-ble_gatt_hvx_notification-under-20-bytes/?answer=55637#post-id-55637

Basically there are two problems:

  1. The GattCharacteristic initLen parameter is randomly named minLen too. If you follow the code all the way through it actually does two things - it is the initial length (not minimum), and it determines whether the characteristic is variable length or not.
  2. The characteristic for nRF51822 code is only variable length if initLen != maxLen. This is really stupid because you might want the initial length to equal the maximum length but still have variable length. It also isn't documented anywhere.

I suggest the following solution:

  1. Add extra constructors with an extra parameter: bool variableLength.
  2. Name initLen to be initLen everywhere consistently (and not minLen).
  3. Deprecate the old constructors.
@rainierwolfcastle
Copy link

ARM Internal Ref: IOTSFW-1159

@rgrover
Copy link
Contributor

rgrover commented Nov 26, 2015

@Timmmm I agree that initLen should not have been renamed as minLen. I also agree that initlen != minLen should not be a criteria for variable-length characteristics.

@andresag01
Copy link

@Timmmm Apologies on the delay for a fix. I have just submitted two pull requests to address the issue (to ble and ble-nrf51822). The GattAttribute.initialLen was removed and we are now using len as the current length of *valuePtr and maxLen as the maximum value of *valuePtr. Therefore, we have removed the accessor getInitialLen() and replaced this with getLength() where required in ble-nrf51822 module.

Notice that currently all attributes are marked as 'variable'. We will be submitting pull requests to deal with this shortly, but it was decided that we would do this separately because it requires changing the constructors of the GattCharacteristic and GattAttribute.

@Timmmm
Copy link
Author

Timmmm commented Nov 27, 2015

Awesome, no need to apologise!

@andresag01
Copy link

@Timmmm I have made the latest changes to allow variable length characteristics by passing a boolean to the constructor. Please see: d71c316.

@Timmmm
Copy link
Author

Timmmm commented Dec 2, 2015

Excellent, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants