arduino / nicla-sense-me-fw

Arduino Nicla Sense ME resources (libraries, bootloader, host pc utilities)
GNU Affero General Public License v3.0
47 stars 27 forks source link

[AE-91] SensorID 70 `SENSOR_ID_DEVICE_ORI_WU` initially gives value of `0.000` regardless of orientation #102

Open aliphys opened 1 year ago

aliphys commented 1 year ago

Describe the Problem

😔 Polling sensor ID 70 SENSOR_ID_DEVICE_ORI_WU gives an inital reading of 0.000 regardless of actual orientation. Additionally, (from a user perspective) the datatype should be int. See table 79 in datasheet.

https://github.com/arduino/nicla-sense-me-fw/blob/b4425275859b2dea1ec22c546851106df9ac8fb5/Arduino_BHY2/src/sensors/SensorID.h#L49

image

To reproduce

/*
Sketch that checks orientation sensor (sensorID 70) (see table 79 of BHI260 datasheet), requesting 100 samples of each
With Putty, log the Serial output as a .txt
@author: Pablo Marquínez, Ali Jahangiri
*/

#include "Arduino_BHY2.h"
int sensors[] = {70};
Sensor* ptrSensor;

void setup() {
  // put your setup code here, to run once:
  Serial.begin(9600);
  while (!Serial)
    ;

  delay(10000);
  BHY2.begin();

  delay(1000);

  checkSensors();

  Serial.println("--------");
  Serial.println("END");
}

void loop() {
  // put your main code here, to run repeatedly:
}

void checkSensors() {
  Serial.println("-------");
  Serial.println("Checking type Sensor");
  int listLength = sizeof(sensors) / sizeof(sensors[0]);
  for (int checkID = 0; checkID < listLength; checkID++) {
    ptrSensor = new Sensor(sensors[checkID]);
    ptrSensor->begin();

    Serial.print("\tChecking ");
    Serial.println(sensors[checkID]);
    for (int i = 0; i < 100; i++) {
      BHY2.update();
      Serial.println(String("\t\tSample n") + String(i) + String(" ") + ptrSensor->toString());
      delay(20);
    }

    ptrSensor->end();
  }
}

Output:

Checking 70
Sample n0 Data value: 0.000

Sample n1 Data value: 0.000

Sample n2 Data value: 3.000

Sample n3 Data value: 3.000

Sample n4 Data value: 3.000

Sample n5 Data value: 3.000

Sample n6 Data value: 3.000

Sample n7 Data value: 3.000

Sample n8 Data value: 3.000

Sample n9 Data value: 3.000

Sample n10 Data value: 3.000

...

Sample n99 Data value: 3.000

Expected behaviour

🙂 Initial value is not zero. Either the correct value is given (e.g. 3), or an output is given that does not correspond to the expected data (e.g. -1) 🙂 Data type is int (so 3 not 3.000)

Arduino CLI version

0.32.3 - https://github.com/arduino/arduino-cli/commit/2661f5d9a68c028df370cd56b4ae7f2a4b651c4c

Operating system

Windows 10

Additional Context

The initial value of multiple sensors (including ) is also zero. However, given that the values of sensor ID SENSOR_ID_DEVICE_ORI_WU are discrete it is difficult to discern if the 0 relates to the sensor warming up (i.e. reading the value from the FIFO buffer) or if it is the actual orientation being portrait upright.

https://github.com/arduino/nicla-sense-me-fw/blob/b4425275859b2dea1ec22c546851106df9ac8fb5/Arduino_BHY2/src/sensors/SensorID.h#L163

Additionally, the value of the sensor orientation is not documented in Arduino documentation. The only way a user may know is through the BHI260 datasheet. Even then, it is not clear if the same conventions are used between the BHI260 datasheet and the Arduino_BHY2 library.

aliphys commented 1 year ago

@marqdevx @i-herrera can you also confirm this behaviour in your evaluations?

bstbud commented 1 year ago

Hi This issue is caused by the asynchronous polling of the sensor. It happens when the first sensor value dump: ptrSensor->toString() is called before the first event even gets to the FIFO and read out. This could be easily solved by waiting some time after the sensor is enabled (begin()). See the delay added below.

void checkSensors() {
  Serial.println("-------");
  Serial.println("Checking type Sensor");
  int listLength = sizeof(sensors) / sizeof(sensors[0]);
  for (int checkID = 0; checkID < listLength; checkID++) {
    ptrSensor = new Sensor(sensors[checkID]);
    ptrSensor->begin();

    delay(1000);//after enabling a sensor, it takes some time before the first sensor event is populated
    Serial.print("\tChecking ");
    Serial.println(sensors[checkID]);
    for (int i = 0; i < 100; i++) {
      BHY2.update();
      Serial.println(String("\t\tSample n") + String(i) + String(" ") + ptrSensor->toString());
      delay(20);
    }

    ptrSensor->end();
  }
aliphys commented 1 year ago

Hello @bstbud 👋 Thank you for looking into this.

🙂 Applying your fix (adding a delay of 1000 ms) does indeed eliminate the initial 0.000 values. Here is the output:

-------
Checking type Sensor
    Checking 70
        Sample n0 Data value: 3.000

        Sample n1 Data value: 3.000

        Sample n2 Data value: 3.000

        Sample n3 Data value: 3.000

        Sample n4 Data value: 3.000

        Sample n5 Data value: 3.000

        Sample n6 Data value: 3.000

        Sample n7 Data value: 3.000

        Sample n8 Data value: 3.000

        Sample n9 Data value: 3.000

        Sample n10 Data value: 3.000
...

        Sample n99 Data value: 3.000

--------
END

I can also confirm that a delay of 100ms is also sufficent to reliably prevent the initial zero value for sensor ID 70.

aliphys commented 1 year ago

There are three problems with this solution:

  1. The User is required to take additional steps to get the non zero values. We want to make use of the BHI260 (and by extension the Nicla Sense ME cc @martab1994 ) as easy as possible with minimal friction
  2. When switching between different sensor IDs, the delay must be re-applied. This creates friction for users who want to use multiple sensors of the BHI260, especially in low-energy solutions cc @akash73
  3. The delay is arbitrary and may not account for variations between the different sensors, especially when the sensor fusion is involved.

Following the call on Monday, is there a solution the Bosch team can provide for this?

aliphys commented 1 year ago

An example for points 2 (switching between sensor IDs) and 3 (seemingly arbitrary delay) in the comment above: this is the output when the sketch calls sensor IDs 69 (non wake up) and 70 (wake up) int sensors[] = {69,70}; without the delay;

-------
Checking type Sensor
    Checking 69
        Sample n0 Data value: 0.000

        Sample n1 Data value: 3.000

        Sample n2 Data value: 3.000

        Sample n3 Data value: 3.000

        Sample n4 Data value: 3.000

        Sample n5 Data value: 3.000
...

        Sample n99 Data value: 3.000

    Checking 70
        Sample n0 Data value: 0.000

        Sample n1 Data value: 0.000

        Sample n2 Data value: 3.000

        Sample n3 Data value: 3.000

        Sample n4 Data value: 3.000

        Sample n5 Data value: 3.000
...

        Sample n99 Data value: 3.000

--------
END

When a delay of 50ms is included, this eliminates the zero value for sensor ID 69 but not 70.


-------
Checking type Sensor
    Checking 69
        Sample n0 Data value: 3.000

        Sample n1 Data value: 3.000

        Sample n2 Data value: 3.000

        Sample n3 Data value: 3.000

        Sample n4 Data value: 3.000

        Sample n5 Data value: 3.000
...

        Sample n99 Data value: 3.000

    Checking 70
        Sample n0 Data value: 0.000

        Sample n1 Data value: 3.000

        Sample n2 Data value: 3.000

        Sample n3 Data value: 3.000

        Sample n4 Data value: 3.000

        Sample n5 Data value: 3.000

        Sample n99 Data value: 3.000

--------
END
bstbud commented 1 year ago

Hi @aliphys

Thanks for your update as to the confirmation that the issue could be resolved by adding proper delays. The delay is just a crude / intermediate solution. As was mentioned, the toString() is a synchronous call, while the sensor data is reported asynchronously, to address this mismatch, one option is to add a member variable _available and function such as available() which returns true only when a new data has been reported and cleared when the toString() is called. option 2 is to add a member function for the class SensorClass, and this function allows the client (the sketch) to register a callback which can for example dumps the values as soon as the new data is reported.

Option 2 is better, I think.

aliphys commented 1 year ago

Hello @bstbud , As mention in the email, could you go ahead with Option 1 Add a variable to the class (e.g. _avaliable), which is true only when new data can be read?

aliphys commented 1 year ago

Modified original sketch to use the .dataAvaliable() method to check prior to polling data from virtual sensor.

Sketch with `sensors[checkID].dataAvaliable()` method ``` /* Sketch that checks orientation sensor (sensorID 70) (see table 79 of BHI260 datasheet), requesting 100 samples of each With Putty, log the Serial output as a .txt @author: Pablo Marquínez, Ali Jahangiri */ #include "Arduino_BHY2.h" int sensors[] = {70}; Sensor* ptrSensor; void setup() { // put your setup code here, to run once: Serial.begin(9600); while (!Serial) ; delay(10000); BHY2.begin(); delay(1000); checkSensors(); Serial.println("--------"); Serial.println("END"); } void loop() { // put your main code here, to run repeatedly: } void checkSensors() { Serial.println("-------"); Serial.println("Checking type Sensor"); int listLength = sizeof(sensors) / sizeof(sensors[0]); for (int checkID = 0; checkID < listLength; checkID++) { ptrSensor = new Sensor(sensors[checkID]); ptrSensor->begin(); Serial.print("\tChecking "); Serial.println(sensors[checkID]); for (int i = 0; i < 100; i++) { BHY2.update(); if(sensors[checkID].dataAvailable()) { Serial.println(String("\t\tSample n") + String(i) + String(" ") + ptrSensor->toString()); delay(20); sensors[checkID].clearDataAvailFlag(); } } ptrSensor->end(); } } ```

This error is produced.

Compilation error: request for member 'dataAvailable' in 'sensors[checkID]', which is of non-class type 'int'

This method should work for all sensorIDs according to

https://github.com/bstbud/nicla-sense-me-fw/blob/b7dc6a60308093febddabf5082360dfbee1045b0/Arduino_BHY2/src/sensors/SensorManager.cpp#L9-L18