Return ReportID being read from the sensor

[ya-mouse]

Current function dataAvailable initiates data read and returns boolean without respect to which kind of data being read.
If the sensor being configured to report different sensors, then dataAvailable() wouldn't provide essential info which sensor were updated.

[ya-mouse]

Ping

[PaulZC]

Hi @ya-mouse,
Thank you for submitting this PR. Apologies for the delay in getting back to you.
Can you please target this PR at the release-candidate branch (as per the contributing guide)? That way we can test it before making changes to the master branch.
Many thanks!
Paul

[guihomework]

Hi, I had a similar need of knowing which data was really updated and forked the code there guihomework@905084e . I was in the process of providing a PR, but saw this one which aims to a similar goal.
My idea was to keep the dataAvailable as is but add new methods to handle the availability (getOnNewXXX , isNewData, resetNewData) of each type of data in a table. Since some data are available at a slower rate, the main loop can process fast data that is always new, but decide to not process non-updated data, in a more fine grained way.
Additionnally I added methods to access all x,y,z data in one go.

Would be nice to get some comments if one or the other idea (or both) should be prepared as a PR, and which feature seem worth keeping.

thanks
Guillaume

[PaulZC]

Hi @guihomework & @ya-mouse ,
If you would like to submit a PR, please go ahead. All that we ask is that you target the release-candidate branch (as per the contributing guide). That way we can test your code before making changes to the master branch.
We always welcome new contributions (so long as they have no backwards-compatibility issues!).
Best wishes,
Paul

[guihomework]

I wanted to open a discussion on what the PR should contain, to not provide a "duplicate" or try to take-over @ya-mouse. Maybe the discussion should be on a new issue, rather than on this PR. @ya-mouse what do you think ?

[ya-mouse]

That consumes a lot of memory while could be stored in simple bit vector:

//Vector of flags to report on newdata for a given report
bool newData[SENSOR_REPORTID_PERSONAL_ACTIVITY_CLASSIFIER+1];

With such a code:
guihomework@905084e#diff-2d4e55457b976ef8dfb343a6b47c7e2aR381-R389

on each packet receive you should call all of the routines that you have expected to be supported instead of handling the current packet type (like my PR do).

[ya-mouse]

@PaulZC, I've updated the base.

[guihomework]

That consumes a lot of memory while could be stored in simple bit vector:

//Vector of flags to report on newdata for a given report
bool newData[SENSOR_REPORTID_PERSONAL_ACTIVITY_CLASSIFIER+1];

With such a code:
guihomework@905084e#diff-2d4e55457b976ef8dfb343a6b47c7e2aR381-R389

on each packet receive you should call all of the routines that you have expected to be supported instead of handling the current packet type (like my PR do).

You are right about the memory consumption.
However, regarding "all" the subroutine call to know if there is data, I don't agree fully, it depends on the implementation. You probably have a mainloop which processes the latest packet and then indeed your PR makes sense. Maybe providing an example how you use your getReadings in the mainloop would help confirm.

I was not processing the latest packet in my mainloop, because I want to adjust the speed at which I process (=send to the PC) the new data in a regular timed interrupt. The mainloop was just calling the library data extraction (with dataAvailable) fast enough and in this interrupt I check if all data I need are there and create a frame (acc, gyro, quat for instance) to send the latest of all of them together to the PC.

With your code, I need to store a boolean vector (or if I optimize memory, a bit array) in my main loop and poll this "external" boolean vector in my code. Will work as well, but I wanted to provide this newData state to every user of the lib because in my opinion the state of the data of the sub-functions of the chip should be available on request from the lib.

Maybe a compromise could be to store a bit array in the lib (in you reworked dataAvailable it will be easy with the return value of getReadings to update the bit array), for later use, and still have your direct answer when doing getReadings.

I would be fine to provide a PR on top of your changes, and I am preparing a different PR regarding the helper functions to retrieve x,y,z values in one call (unrelated to new data available then)

[ya-mouse]

The sample code is:

void loop()
{
   switch (myIMU.getReadings())
   {
      case SENSOR_REPORTID_ACCELEROMETER: {
         auto x = myIMU.getAccelX();
         ...
         rgAccel.push(myIMU.getTimeStamp(), x, y, z);
      }
      break;

      case SENSOR_REPORTID_ROTATION_VECTOR:
      case SENSOR_REPORTID_GAME_ROTATION_VECTOR: {
         auto quatI = myIMU.getQuatI();
         ...
         rgQuant.push(myIMU.getTimeStamp(), quatReal, quatI, quatJ, quatK);
      }
      break;

      case 0:
         // No data update or unsupported Command Report response type in the library
         break;

      default:
         // Unhandled Input Report
         break;
   }
}

What is important: getTimeStamp returns microseconds since reading was taking.
In your implementation how did you know when the data is stale? By default the code resets newDataFlag. Thus, the codeflow in loop looks like:

void loop()
{
   if (!myIMU.dataAvailable()) {
      continue;
   }

   float x;
   float y;
   float z;
   uint8_t acc_accuracy;
   float quantReal;
   float quantI;
   float quantJ;
   float quantK;
   float rad_accuracy;
   uint8_t quat_accuracy;

   // Only one of the next routines will evaluate after readings above.
   // The input packet is only one per reading call.
   // There is no reason to read several input packets and the evaluate them at once while the input packets might be produced at different rates and some data might be missed.

   if (myIMU.getOnNewLinAccel(&x, &y, &z, &acc_accuracy)) {
      rgAccel.push(/* */, x, y, z);
   }
   if (myIMU.getOnNewQuat(&quantI, &quantJ, &quantK, &quantReal, &rad_accuracy, &quant_accuracy)) {
      rgQuat.push(/* getTimeStamp is not relevant */, quant...);
   }
   ...
}

After each getOnNewLinAccel the corresponding vector value would be reset. By default the routines are not re-entrant and the purpose of the getXXX to return the actual data by many sequential calls as you want, thus, one call that modifies the state by default is confusing. Like single calls to getAccelX(). IMO, such routines looks specific for the one-off use cases, but not for the library.

If you have a 10 milliseconds reading period or less? I'm not counting on SPI interface that requires an exact reaction on the packet (by interrupt pin) within a strict deadline, otherwise BNO080 will be reset by its watchdog (BNO088 has ability to re-try sending the data, but still requires a real-time operation).

In case of sending frame to PC just keep a Ring Buffer long enough (you can have separate RingBuffers that stores different kind of data with timestamps) with several data series and poll them periodically from PC. Or even push to PC.

Having one routine to get all values at once might be a good idea. Keep this change in separate PR.

The boolean vector stuff, IMO, is an implementation details for the specific project. I would keep it within it. Not a big deal :)

PS. I had another change that returns TimeStamp with current local system's microseconds right at the time of parsing the packet minus the packet's measuring microseconds time. The code is naive and didn't account on microseconds wrap, so, incomplete =)

[guihomework]

Hi @ya-mouse

thank you for taking the time to demonstrate your idea and compare it to mine. I repeat your idea is fine. I am trying to see if some additional stuff could help others.

I was not using getTimeStamp at my side (more on that lower) so I missed your very good point about knowing when the data was acquired.
The fact that your main loop has to take care of storing the timestamp and the data in your ring buffers is a pity, at least the timestamp should be available per data acquired IMHO, not only globally for the last report. In the current situation, and with your PR, everybody has then to implement that timestamp storage, when the timestamp of the relevant data (for instance rawLinAccelX,Y,Z) should be accessible while one gets the individual data (even if it gets overwritten by newer data + newer timestamp). Your method permits, in your mainloop, to not miss a data, and store all. Nice.

Your idea of my usage is a little bit off. For my system I need a full frame (lin acc, gyro, and quat), and the frame speed is limited to the slowest data. Hence, at the time I did 3 getOnNewXXX for a full new frame, maybe some the fastest data had already been missed but it was replaced by fresher ones, and my newData was then true for all 3 types. This way I was sure I had the most fresh data possible for the combination of 3 types of data. For me the timestamp could not really be combined for my frame, even if I had stored it for each type, so I decided to not use it.

here is the code that is currently running in one of our device. The interrupt is at 1ms (for our tactile sensors through external ADCs overs SPI), so faster than the refresh rate of the BNO08x set to 2 ms. While re-reading my code, I realize I am not doing what I thought I had coded and reported earlier. I apologize. I am not calling dataAvailable in the mainloop as fast as possible.
I remember now I coded differently due to shared SPI bus in my system, that would have led to overhead in handling the SPI bus sharing between the main loop and the interrupt (or 2 interrupts and using priorities etc...). I think this was a compromise in the rush to prepare this firmware and never got reworked :-(
https://github.com/ubi-agni/TactileGlove/blob/305267a3dd074f97fb07f552b273b7924b2c0f82/firmware/Teensy3.2/glove_64adcMax11131_16bend_imuBNO085_SP/glove_64adcMax11131_16bend_imuBNO085_SP.ino#L311

Hence, for 3 new data types to be "new" at the same time, the shortest period would be 3 times calling the dataAvailable, which means 3ms in my case. This explains my slower rate that I was seeing (but sufficient for the IMU). A rework is necessary it seems.

In the best case, the SPI handling in my code should be with interrupts on new data of the BNO08x, and take care to store the data locally, until all data are the "most fresh" to create a full frame. Your code permits that.

I think providing an example based on the code you shared in the conversion as part of the PR could be good. Ideally a second example for SPI with interrupt, and handling the storage at user code side could be nice as well (I can do that later as this would be required for my code anyway).

Regarding the timestamp storage for different data types, if you concur, this could be a new PR too if need be

thanks,
Guillaume

[ya-mouse]

Let's keep the library simple. The only purpose of the library is to:

1. Read data / parse packets if its available and return a statement if anything on the plate (e.g. through dataAvaliable or getReadings)
2. Provide API to get actual readings for desired sensors.

Calls to dataAvailable reads only one packet at a time. Thus, in your loop you can only read out one sensor type and several if statements has non-sense over there (they could be rewritten in if-else-if way):
https://github.com/ubi-agni/TactileGlove/blob/305267a3dd074f97fb07f552b273b7924b2c0f82/firmware/Teensy3.2/glove_64adcMax11131_16bend_imuBNO085_SP/glove_64adcMax11131_16bend_imuBNO085_SP.ino#L316-L331
The series of calls OnNew resets the boolean vector, thus, there is no purpose to use it if you just get the proper sensor type from the sensor readings (a-la getReadings). All other values in the vector would be always false and only one flag will always flip/flap in the loop.

That's why switch statement would instantly answer if there is anything and if so, then what type to handle. At the application level you should track which values are received to complete the frame. That's not a Library's purpose.

[guihomework]

Yes to all you wrote about my code, I noticed exactly what you commented on my code when I myself read it again. Currently only one "if" is valid at a time and makes no sense (but at least the data was stored once in my variables and no further memory copy would be done)
However, if I had coded data collection in an interrupt callback handling the "asynchronous" call to dataAvailable of the BNO08X SPI without consuming the data (keep it in the lib), then my flag would make sense in the "precise timed interrupt" I have, doing the concatenation of data in my frame, it would know if all data was asynchronously collected and then all "if" would answer true. I had this in mind initially but did not code it this way yet. Sorry for the confusion.

The thing is, the current lib does not do only 1. and 2. that you suggest, it also does
3. stores the actual readings (but not the yet their associated timestamp), until a newer overwrites it.

because there is a 3. existing, I thought augmenting it with the "state" of the data (= was it "consumed" or not...)

Your solution is keeping the lib simple and not storing the state of the data (needs to be consumed when it is there to store the state at the mainloop side), so my discussion is only about a choice what should the library store.

If I follow you, 3. would have to be removed as well from the lib and the mainloop would necessarily have to process the data when it is there, including storing, keeping track of flags after each getReadings (which you do) and then remove all those memory usage about storing all data types in the lib.

The choice of what to store in the lib is a question that the maintainer should decide on. @PaulZC do you want to comment on your vision of what should be stored in the lib ? (newdata state , data type individual timestamp ?)

I suppose for backward compatibility 3. will stay, and the additional getReadings of your PR is just in order to bypass the internals that do not store all info yet, while my 'clumsy' idea was to add some kind of info about the state.

thanks for your kind answers and comments.
I stop bothering you, I will probably test your PR on my side very soon and provide the second PR regarding get xyz data in one go.

[ya-mouse]

Don't excuse :))

The precise interrupt-driven handling is another story and perhaps might lead to different design of the library. My opinion, that the lib is not a silver bullet, but to demonstrate how to interact with the sensor using either i2c or spi. You probably might want to skip the Q-numbers conversion to float and just store original values from the sensor. Later you can convert them to doubles (e.g.on PC)

For the high rate application it might be worth just to consume parts from the library code to efficiently handle new data packets.

In your case you just handle getReadings in interrupt handler, store data for the new frame and tracks completeness by bit flags. Once all type of data is collected, then release the frame (e.g.store in data collection/ringbuffer/whereever), reset the bit flag and re-init the frame for the new data. This will keep the interrupt handler short.

It would be great to have a one routine per sensors type to copy measurements.

[ya-mouse]

@PaulZC , ping :)

[PaulZC]

Are you guys finished?!
:-D

[guihomework]

Hi,
I am not sure it is my duty to provide a test example for this PR, and I had no time yet to test this PR on my side. It is on my week todo list as I gathered hardware on friday to test, but have to solder it first.
Are you waiting for me to "review" this in a use-case ?
I think we were finished on this discussion.

[PaulZC]

Hi @ya-mouse ,
Merging... Thank you!
I'll merge into master as soon as I've tested the code.
Thanks again,
Paul

[PaulZC]

Hi @ya-mouse ,
Just for completeness, can you please add getReadings to keywords.txt?
(You can add it underneath dataAvailable. There needs to be a tab character between getReadings and KEYWORD2, not spaces.)
Thanks!
Paul

[guihomework]

@PaulZC before this gets released, I wanted to confirm I am planning to prepare a PR with an example for the getReadings combined with the multiple access #56, maybe even with an interrupt if I manage, as I need that for my project and could test the feasibility by deriving from example18

[PaulZC]

Hi @guihomework ,
OK - that is great. Thank you. I will wait until your example is ready before I release v1.1.9.
Please try to make the example "generic" - so it will run on any board (including our old friend the Arduino Uno if possible!).
Best wishes,
Paul

[guihomework]

@PaulZC FYI, working on that right now. I found an arduino I can use too for my tests.