Skip to content
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

bhy2_get_and_process_fifo does not work #2

Open
sullivanmj opened this issue Feb 21, 2022 · 10 comments
Open

bhy2_get_and_process_fifo does not work #2

sullivanmj opened this issue Feb 21, 2022 · 10 comments

Comments

@sullivanmj
Copy link

sullivanmj commented Feb 21, 2022

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 11381338 and 11391339 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.

@robcazzaro
Copy link

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;
}

@sullivanmj
Copy link
Author

sullivanmj commented Mar 9, 2022

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.

@robcazzaro
Copy link

robcazzaro commented Mar 9, 2022

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)

@sullivanmj
Copy link
Author

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.

@robcazzaro
Copy link

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

@sullivanmj
Copy link
Author

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

@robcazzaro
Copy link

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)

@sullivanmj
Copy link
Author

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.

@dariosortino
Copy link

Unrelated comment:

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

@robcazzaro
Copy link

robcazzaro commented Aug 26, 2022

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 robcazzaro/nRF52_BHY2_API#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

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

No branches or pull requests

3 participants