BHI2xy_SensorAPI icon indicating copy to clipboard operation
BHI2xy_SensorAPI copied to clipboard

bhy2_get_and_process_fifo does not work

Open sullivanmj opened this issue 3 years ago • 10 comments

bhy2_get_and_process_fifo does not fire callbacks that were added with bhy2_register_fifo_parse_callback

The reason for this is that the callback is not properly retrieved in the get_callback_info function. There are bugs on lines ~~1138~~1338 and ~~1139~~1339 of bhy2.c. Instead of:

info->callback = NULL;
info->callback_ref = NULL;

those lines should be:

info->callback = dev->table[sensor_id].callback;
info->callback_ref = dev->table[sensor_id].callback_ref;

When this change is made, then callbacks that were registered with bhy2_register_fifo_parse_callback are called when the appropriate data is in the FIFO, when bhy2_get_and_process_fifo is called.

sullivanmj avatar Feb 21 '22 22:02 sullivanmj

Not the owner of this repository, but I was looking at open issues and found yours, The version of the library checked in this repository on May 13 2020, has the following starting at line 1147. It looks as if you are referring to a different version of the library, given the different coding conventions and line numbering

I recently ported the library to a Nordic microprocessor, and callbacks work as expected

int8_t bhy2_register_fifo_parse_callback(uint8_t sensor_id,
                                         bhy2_fifo_parse_callback_t callback,
                                         void *callback_ref,
                                         struct bhy2_dev *dev)
{
    int8_t rslt = BHY2_OK;

    if ((dev == NULL) || (callback == NULL))
    {
        rslt = BHY2_E_NULL_PTR;
    }
    else
    {
        dev->table[sensor_id].callback = callback;
        dev->table[sensor_id].callback_ref = callback_ref;
    }

    return rslt;
}

robcazzaro avatar Mar 09 '22 21:03 robcazzaro

Ah, looks like I made a mistake in writing the line numbers in my original comment. I meant to write "lines 1338 and 1339" instead of "1138 and 1139". I've updated it now.

Thanks for catching that.

Interesting that the callbacks are working for you but not for me. I ported the code for use on a SAM D51.

sullivanmj avatar Mar 09 '22 21:03 sullivanmj

But... those lines (1338, etc) are for the get_callback_info(), which is not supposed to set any callback. Not for the bhy2_register_fifo_parse_callback() as your issue originally refers to

Also, the current version of the library uses

        dev->table[sensor_id].callback = callback;
        dev->table[sensor_id].callback_ref = callback_ref;

and not

info->callback = dev->table[sensor_id].callback;
info->callback_ref = dev->table[sensor_id].callback_ref;

you have info-> and the current code has dev->

Let me ask again: you really using the library in this repository or a different version?

Mine works because the code is written as you suggest should be written :) but it has been that way for almost two years (in this repository)

robcazzaro avatar Mar 09 '22 21:03 robcazzaro

get_callback_info() is called by parse_fifo() in the same file. parse_fifo() is supposed to get the registered callbacks via that function, and then invokes them. Perhaps we're using the library in a different way?

I had to make this change to get the example program in euler.c, version 5b26dc4.

sullivanmj avatar Mar 09 '22 21:03 sullivanmj

I'm not making myself clear, sorry.

Try downloading this repository and compare it with your version of the same library. The code you refer to, doesn't exist in this repository. I believe you are using a version you downloaded from somewhere else.

I understand how the code works. I ported it to a Nordic processor (Arm M4 core), and the callbacks work as expected (from bhy2_get_and_process_fifo() which in turns calls parse_fifo(). The code in this repository correctly registers all callbacks

robcazzaro avatar Mar 09 '22 22:03 robcazzaro

My initial complaint was that the callbacks that were registered with bhy2_register_fifo_parse_callback were not subsequently invoked. Not that they were not correctly registered - because they are indeed correctly registered. The problem that I have seen is that once those callbacks are registered, they are not correctly retrieved.

I cloned this repository directly from GitHub using git, and the code that I mentioned exists in this repository as I've specified:

https://github.com/BoschSensortec/BHY2-Sensor-API/blob/5b26dc4802f500f75e1086c2a156e3ecf800560c/bhy2.c#L1338 https://github.com/BoschSensortec/BHY2-Sensor-API/blob/5b26dc4802f500f75e1086c2a156e3ecf800560c/bhy2.c#L1339

sullivanmj avatar Mar 09 '22 22:03 sullivanmj

As I said, it's working for me, for all the virtual sensors I register and initialize

My reading of the get_callback_info() function, is that it "nulls" the callback pointers only if one of these two conditions is met

if ((sensor_id >= BHY2_SPECIAL_SENSOR_ID_OFFSET) && (info->event_size == 0)) or if ((sensor_id == 0) && (info->event_size == 0))

In both those cases, you want the callbacks to be NULL (BHY2_SPECIAL_SENSOR_ID_OFFSET is 245, so higher than any standard virtual sensor

in all the other cases, the callback pointers are retained from the previous registration, and invoked by line 1481 in parse_fifo() info.callback(&data_info, info.callback_ref);

Your proposed fix will not help in the normal code flow, and actually risks causing problems to the proper code execution (once again, I'm saying this as a semi-clueless user of this library and in the spirit of trying to help)

robcazzaro avatar Mar 10 '22 00:03 robcazzaro

I appreciate the discussion on this. I'm also semi-clueless on this library. I was just posting the change that I made in order to make to get the example program in euler.c to work. Perhaps there is something else wrong with that example.

sullivanmj avatar Mar 14 '22 14:03 sullivanmj

Unrelated comment:

@robcazzaro I need also to implement this in a nordic micro. Could you share with me what you have done so far?

dariosortino avatar Aug 26 '22 12:08 dariosortino

UPDATE: nRF52 port is now available here https://github.com/robcazzaro/nRF52-BHY2-Sensor-API

@dariosortino I never took the time to clean things up and post it. But I should :)

To give you at least a starting point, I created a placeholder project and added an issue where you can see what I did and ask questions. I don't want to sidetrack this issue with unrelated content. Please feel free to add comments/questions to https://github.com/robcazzaro/nRF52_BHY2_API/issues/1

For what is worth, it's working really well and even if the lack of documentation is annoying, the Bosch BHY2 library is well written and functional. I have not found any bug yet

robcazzaro avatar Aug 26 '22 16:08 robcazzaro