drivers: gnss: allow same callback

[PedroSAndre]

drivers: gnss: allow same callback

The current implementation of GNSS_DATA_CALLBACK_DEFINE and
GNSS_SATELLITES_CALLBACK_DEFINE only use the provided _callback to
uniquely identify the entry that will be created. This implies that the
same callback cannot be registered twice, even if the device it is being
attached to is different.

In order to allow the same callback to be registered several times, the
ideal would be to also use _dev in combination with _callback to
uniquely identify the generated entry. But this would require getting the
node_id of the device, hence breaking the current API. In order to avoid
that, use COUNTER to avoid collisions.

gnss: rtk: fix multiple instances of same device

When multiple instances of the same type of RTK enabled GNSS device are
instantiated on the same board, there will be a collision with their RTK
data callbacks. The u-blox F9P module is a clear example of that, where the
following compilation error is reported:

 error: redefinition of '_gnss_rtk_data_callback__f9p_rtk_data_cb'

Similar to the solution used at d36a0fc (drivers: gnss: allow same
callback), use __COUNTER__ instead of _callback in the generated entry
to avoid collisions.

[github-actions]

Hello @PedroSAndre, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[bjarki-andreasen]

The way to reuse the callback is to set _dev to NULL, the _dev is an optional filter, so if NULL is provided, the callback will get called multiple times, once for each _dev, if the app wants to filter further, if (dev == DEVICE_DT_GET(node)) { or similar can be used within the callback.

The solution in this PR works as well, but it should not be strictly necessary

[PedroSAndre]

The way to reuse the callback is to set _dev to NULL, the _dev is an optional filter, so if NULL is provided, the callback will get called multiple times, once for each _dev, if the app wants to filter further, if (dev == DEVICE_DT_GET(node)) { or similar can be used within the callback.

The solution in this PR works as well, but it should not be strictly necessary

I was not aware of the NULL use case, thank you for clarifying @bjarki-andreasen.

Considering that, I do agree that the first commit can be considered not strictly necessary. Despite this, I still believe there is a point in including these changes.

First of all, passing NULL, beyond not being super intuitive, can make the callback logic more complicated without a real need for it. Furthermore, it is also more intuitive, in my opinion, to use the received dev handle in the callback instead of filtering in the callback itself.

It is also relevant to note that this does not break any of the old functionality. If the user wants to still provide NULL and then filter in the callback, that is still perfectly possible.

For the second commit, I think using NULL to fix the RTK use case for the F9P is not possible since the driver cannot be aware of how many instances of the device will be defined and what their properties will be, so no filtering logic is possible in the driver callback itself.

[bjarki-andreasen]

The way to reuse the callback is to set _dev to NULL, the _dev is an optional filter, so if NULL is provided, the callback will get called multiple times, once for each _dev, if the app wants to filter further, if (dev == DEVICE_DT_GET(node)) { or similar can be used within the callback.

The solution in this PR works as well, but it should not be strictly necessary

I was not aware of the NULL use case, thank you for clarifying @bjarki-andreasen.

Considering that, I do agree that the first commit can be considered not strictly necessary. Despite this, I still believe there is a point in including these changes.

First of all, passing NULL, beyond not being super intuitive, can make the callback logic more complicated without a real need for it. Furthermore, it is also more intuitive, in my opinion, to use the received dev handle in the callback instead of filtering in the callback itself.

It is also relevant to note that this does not break any of the old functionality. If the user wants to still provide NULL and then filter in the callback, that is still perfectly possible.

For the second commit, I think using NULL to fix the RTK use case for the F9P is not possible since the driver cannot be aware of how many instances of the device will be defined and what their properties will be, so no filtering logic is possible in the driver callback itself.

For device drivers, we should add GNSS_DT_INST variants to define unique names for callbacks, these use the devicetree ordinal to generate a unique name, and the struct device is gotten from devicetree as well, so just inst/node_id + callback:

GNSS_DT_DATA_CALLBACK_DEFINE(node_id, callback)

[PedroSAndre]

For device drivers, we should add GNSS_DT_INST variants to define unique names for callbacks, these use the devicetree ordinal to generate a unique name, and the struct device is gotten from devicetree as well, so just inst/node_id + callback:

GNSS_DT_DATA_CALLBACK_DEFINE(node_id, callback)

Thank you for your feedback once again @bjarki-andreasen.

I believe I understand where you are coming from, but won't adding macros specific for device drivers be a bit unnecessary? Since the GNSS_*_CALLBACK_DEFINE macros can already handle the device driver use case with these proposed changes, I see no reason to add more macros to achieve it. In my mind at least, it seems to add complexity and split the implementation between device drivers and user-space without a tangible benefit.

But I am probably missing some context as to why adding more macros is ideal, so please feel free to disagree :).

[bjarki-andreasen]

For device drivers, we should add GNSS_DT_INST variants to define unique names for callbacks, these use the devicetree ordinal to generate a unique name, and the struct device is gotten from devicetree as well, so just inst/node_id + callback:
GNSS_DT_DATA_CALLBACK_DEFINE(node_id, callback)

Thank you for your feedback once again @bjarki-andreasen.

I believe I understand where you are coming from, but won't adding macros specific for device drivers be a bit unnecessary? Since the GNSS_*_CALLBACK_DEFINE macros can already handle the device driver use case with these proposed changes, I see no reason to add more macros to achieve it. In my mind at least, it seems to add complexity and split the implementation between device drivers and user-space without a tangible benefit.

But I am probably missing some context as to why adding more macros is ideal, so please feel free to disagree :).

By device drivers, I don't just mean within them, consider

const struct device *gnss_dev_0 = DEVICE_DT_GET(DT_NODELABEL(gnss0));
const struct device *gnss_dev_1 = DEVICE_DT_GET(DT_NODELABEL(gnss1));

We use the devicetree to tie a struct to a specific instance of something, with this PR, defining the callback looks like this:

my_handler_gnss_0(const struct device *dev, const struct device *dev, const struct gnss_data *data)
{
}

my_handler_gnss_1(const struct device *dev, const struct device *dev, const struct gnss_data *data)
{
}

GNSS_DATA_CALLBACK_DEFINE(DEVICE_DT_GET(DT_NODELABEL(gnss0)), my_handler_gnss_0);
GNSS_DATA_CALLBACK_DEFINE(DEVICE_DT_GET(DT_NODELABEL(gnss1)), my_handler_gnss_1);

There are only two different usages of the GNSS_<type>_CALLBACK_DEFINE macros, define with no device filter, so its called for every device, or define to only get a callback from one specific device taken from the devicetree. Having two macros for each specific purpose is clearer and more concise than reusing one

my_handler_gnss_any(const struct device *dev, const struct device *dev, const struct gnss_data *data)
{
}

my_handler_gnss_0(const struct device *dev, const struct device *dev, const struct gnss_data *data)
{
}

/* Could be simplified to not take the NULL argument  */
GNSS_DATA_CALLBACK_DEFINE(NULL, my_handler_gnss_any);
GNSS_DT_DATA_CALLBACK_DEFINE(DT_NODELABEL(gnss0), my_handler_gnss_0);

and a driver would use the inst variant:

GNSS_RTK_DT_INST_DATA_CALLBACK_DEFINE(inst, my_handler_gnss_##inst);

We get rid of the redundant DEVICE_DT_GET and DEVICE_DT_INST_GET calls within the macros this way, its not functionally different, its just more consise

I can agree to this PR as is, just to not break anything, we can add the DT based macros later, and replace uses of the current macro then.

[PedroSAndre]

We get rid of the redundant DEVICE_DT_GET and DEVICE_DT_INST_GET calls within the macros this way, its not functionally different, its just more consise

I can agree to this PR as is, just to not break anything, we can add the DT based macros later, and replace uses of the current macro then.

Thank you again for your feedback @bjarki-andreasen.

Since the idea is for us to move into the DT based macros anyway, I have taken the liberty of implementing those changes in #99208. Considering those are the ideal solution, there is no point in merging the changes in this PR, so I will close it for now if that is okay with everyone :).