Skip to content

Add cast for compatibility with the next version of CurieBLE #335

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

Merged
merged 1 commit into from
Mar 2, 2017

Conversation

sandeepmistry
Copy link
Contributor

Arduino and Intel are working on a new API for CurieBLE, progress can be seen here: arduino/ArduinoCore-arc32#337

There's a backwards compatibility layer with the existing BLEPeripheral based API, so for the most part no code changes will be needed for CurieBLE users once they upgrade.

However, we've overloaded BLECharacteristic::setEventHandler(...) with the following:

typedef void (*BLECharacteristicEventHandler)(BLEDevice bledev, BLECharacteristic characteristic);

typedef void (*BLECharacteristicEventHandlerOld)(BLECentral &central, BLECharacteristic &characteristic);

    void setEventHandler(BLECharacteristicEvent event, 
                         BLECharacteristicEventHandler eventHandler);
    void setEventHandler(BLECharacteristicEvent event, 
                         BLECharacteristicEventHandlerOld eventHandler);

So there's a tiny casting change needed in Firmata need for compatibility, only because a NULL value is passed into setEventHandler. For now we think adding a cast is the best solution to move forward, but are open to other ideas.

cc/ @facchinm @SidLeung @sgbihu @noelpaz @russmcinnis

@facchinm
Copy link
Contributor

This patch is needed to pass the backwards-compatibility test for upcoming CurieBLE v2 library; however, since the APIs are not yet super stable, I'd recommend Intel team to run the test suite with this patch applied, while @soundanalogous should wait before merging it until the library is out (or at least the APIs are stable and merged in https://github.com/01org/corelibs-arduino101 mainline)

@soundanalogous
Copy link
Member

I'm waiting to merge this until the CurieBLE v2 library is ready. Can someone on the Arduino or Intel side notify me when there is a CurieBLE RC is available.

@sandeepmistry
Copy link
Contributor Author

@soundanalogous there's an RC of v2 available now, @facchinm has prepared some instructions on the Arduino forum: http://forum.arduino.cc/index.php?topic=443728.0

However, my initial testing did not go well (sketch compiles with this patch, but something is crashing), I've opened arduino/ArduinoCore-arc32#377 with Intel to track the problem. A member of the Arduino community has also opened arduino/ArduinoCore-arc32#387 to let Intel know about the compile error that this PR addresses.

Let me follow up with Intel in the next week or two, then we can let you know when things are stable for you to test.

@soundanalogous
Copy link
Member

It's sounds like this has been resolved per the thread in the corelibs-arduino101 library. I'm travelling for the next week so I won't have time to test this on HW until I get back. If this needs to be merged earlier and someone can verify that the patch works and is backwards compatible the please verify. Otherwise I'll get to it sometime during the week of 2/20.

@sandeepmistry
Copy link
Contributor Author

I've been testing an end to end scenario with Node.js (https://gist.github.com/soundanalogous/927360b797574ed50e27) and the latest CurieBLE master, things are still not functional for me.

After debugging for a bit found another CurieBLE issue: arduino/ArduinoCore-arc32#444

@soundanalogous I'll keep you updated on any further testing we do, however as I mentioned before, I don't expect any more changes will need to be made to the Arduino Firmata library.

@sandeepmistry
Copy link
Contributor Author

@soundanalogous based on my testing in arduino/ArduinoCore-arc32#377 (comment), this should be good to merge now.

@soundanalogous
Copy link
Member

This is still backwards compatible with older version of CurieBLE right?

@sandeepmistry
Copy link
Contributor Author

Correct, I was actually testing the PR with v1.0.7 in arduino/ArduinoCore-arc32#377 (comment) also.

@soundanalogous
Copy link
Member

Great! Thanks for all of your help with this!

@soundanalogous soundanalogous merged commit 947784f into firmata:master Mar 2, 2017
@SidLeung
Copy link

SidLeung commented Mar 2, 2017

@sandeepmistry , I should pull this PR in for the upcoming release, shouldn't I?

@SidLeung
Copy link

SidLeung commented Mar 2, 2017

Oh! It's done. Pardon me.

@sandeepmistry
Copy link
Contributor Author

@soundanalogous thanks for merging!

Do you know when the next version of the Firmata library is schedule to be released?

@soundanalogous
Copy link
Member

I could cut a bugfix release this weekend.

@sandeepmistry
Copy link
Contributor Author

A new bug fix release at your convenience would be great!

@soundanalogous
Copy link
Member

@sandeepmistry
Copy link
Contributor Author

Thank you! I've updated the IDE side to use v2.5.5 in the hourly builds and in the next release (arduino/Arduino@197fe6d).

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

Successfully merging this pull request may close these issues.

4 participants