Skip to content

Update BLE examples #375

New issue

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

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

Already on GitHub? # to your account

Closed
wants to merge 4 commits into from
Closed

Conversation

sandeepmistry
Copy link
Contributor

  • Make peripheral examples consistent with the v1.0.7 ones
  • Move some non-Arduino style examples to test folder
  • Add some missing keywords to keywords.txt
  • Simplify led_control example by using BLE.scanForUuid(...)

@SidLeung
Copy link
Contributor

SidLeung commented Jan 5, 2017

This PR is under internal testing and review. Will merge to master upon completion.

@russmcinnis
Copy link
Contributor

Testing results for BLE. I only tested BatteryMonitor_Central.ino from the test directory. I tried to exercise everything from Sandeep's notes. I had to make a change to BatteryMonitor_Central.ino - it is in the test directory.
BLE.scanForName("BatteryMonitorSketch"); change to BLE.scanForName("BatteryMonitor");
if ( peripheral.localName() == "BatteryMonitorSketch") { change to if ( peripheral.localName() == "BatteryMonitor") {

The other suggestion is to change the IMU notification example to use useful data by using the readMotionSensorScaled() function instead of the readMotionSensor() function.

void readMotionSensor(int& ax, int& ay, int& az, int& gx, int& gy, int& gz);
void readMotionSensorScaled(float& ax, float& ay, float& az, float& gx, float& gy, float& gz);

Here are the results of the sketches tested.
• IMU Ble sketches are in the test folder and they compile and run. I can see the IMU data changing when I move the notification board, but I am not interpreting what the raw data should be.
• BatteryMonitor_Central is in the test directory as expected, but in order to run with BatteryMonitor a change to the local name is needed. Central is searching for “BatteryMonitorSketch” (in two places) while BatteryMonitor is setting the local name to “BatteryMonitor”. After changing the name to “BatteryMonitor” in the central sketch they run properly.
• Led_control – LED, CallbackLED – compiles and runs properly
• ButtonLED compiles and runs properly
• MIDIBLE – compiles and uploads – did not test
• Peripheral_explorer – compiles and runs properly
• Scan – compiles and runs properly
• scan_callback – compiles and runs properly
• sensortag_button – compiles and runs properly

@russmcinnis
Copy link
Contributor

Here is an updated copy of the Ble IMU central and notification sketches that make use of CurieIMU.readMotionSensorScaled(imuAccel.ax, imuAccel.ay, imuAccel.az, imuGyro.gx, imuGyro.gy, imuGyro.gz);
so that the data makes more sense

It looks like these examples are still in the test directory because they are not in Arduino format and are using pointers so the notification program I updated still uses pointers. I had to send the IMU data in two chunks since Ble is only taking 20 bytes and the 6 floating point values from the scaled function are more than 20 bytes. I have attached the sketches.

IMUBle_Examples.zip

@sandeepmistry
Copy link
Contributor Author

@russmcinnis

The other suggestion is to change the IMU notification example to use useful data by using the readMotionSensorScaled() function instead of the readMotionSensor() function.

This is ok with me, in general the items in the test folder still need to be for "Arduino style".

@noelpaz
Copy link
Contributor

noelpaz commented Jan 13, 2017

Are we done with the changes and suggestions?

@sandeepmistry
Copy link
Contributor Author

I don't have any more changes for now.

@sandeepmistry
Copy link
Contributor Author

@SidLeung I've re-based this.

@SidLeung
Copy link
Contributor

SidLeung commented Mar 4, 2017

This PR will be merge to main trunk for the upcoming release (Deneb).

@SidLeung SidLeung assigned sandeepmistry and unassigned SidLeung Mar 4, 2017
@russmcinnis
Copy link
Contributor

@sandeepmistry I was going to make the change to use scaling. Are we ok with using pointers like this for Arduino style? I don't see another way around it since value() is returning an address.
const unsigned char *cvalue = bleImuChar.value();
const imuFrameType *value = (const imuFrameType *)cvalue;
Serial.print("\r\nCharacteristic event, written: ");
Serial.print(value->index);
Serial.print("\t");
Serial.print(value->slot[0]);
If so then I would probably create the following structure to work with the scaling functions:
typedef struct {
int index; // for tracking data, instantaneous accerometer and gyro data will have the same index
bool accelData; // true if accelerometer data
float ax, ay, az;
} IMUACCELDATA;

typedef struct {
int index; // for tracking data, instantaneous accerometer and gyro data will have the same index
bool accelData; // false if gyro data
float gx, gy, gz;
} IMUGYRODATA;

// Buffers to hold IMU data
IMUACCELDATA imuAccel;
IMUGYRODATA imuGyro;

@SidLeung SidLeung requested a review from russmcinnis March 20, 2017 19:35
@sandeepmistry
Copy link
Contributor Author

@russmcinnis

I was going to make the change to use scaling. Are we ok with using pointers like this for Arduino style?

Not really, let's stick to moving the example to the test folder for now. We can continue the discussion later. While it's in the test folder, it's excluded from the release so non-Arduino style is ok.

For future reference here's an API Reference @tigoe has linked to earlier: https://www.arduino.cc/en/Reference/APIStyleGuide

@SidLeung
Copy link
Contributor

@russmcinnis , let's keep your changes in the test directory for Deneb.RC2 release.

@noelpaz @russmcinnis , please do a check on this PR prior to merging.

@SidLeung SidLeung requested a review from bigdinotech March 20, 2017 22:55
@SidLeung
Copy link
Contributor

@bigdinotech , please review the code changes.

@SidLeung SidLeung removed the request for review from bigdinotech March 20, 2017 23:39
@SidLeung
Copy link
Contributor

Jira 872 and #482 addressed this request.

@sandeepmistry
Copy link
Contributor Author

Closing this, as the changes have been replicated by PR #482 which has been merged.

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

Successfully merging this pull request may close these issues.

5 participants