drivers: sensors: akm09918: make submit function more unblocking

[FlorianWeber1018]

The driver now does not wait for the completion of a measurement in the submit function.
Instead it schedule the fetch and the completion of the submission queue entry as delayed work to the system work queue.

[ubieda]

Hi @FlorianWeber1018, may you check if #74435 already addresses your needs?

[FlorianWeber1018]

Hi @ubieda I have tested before 10 minutes but your PR does not solve my problem. (debugged with logic analyzer)
My Problem with the api is that i must read out 4 sensors but infact that the driver waits for measurement completion before the next measurement get started there could only be one conversion / RTIO at a time. maybe you can split up the measurement start and fetch in different work and start the fetch delayed to the start instead let the work sleep for the time the conversion needs. I think then my issue could also be solved by your PR.

But i am with you that your approach (using a dedicated workqueue instead of the system workqueue) is the more elegant way. After your PR is merged i will rebase and adopt my changes.

[FlorianWeber1018]

CI is failing because the system workqueue has the same priority (-1) than the test in sensors.generic_test.
After #74676 is merged CI should pass.

[ubieda]

My Problem with the api is that i must read out 4 sensors but infact that the driver waits for measurement completion before the next measurement get started there could only be one conversion / RTIO at a time. maybe you can split up the measurement start and fetch in different work and start the fetch delayed to the start instead let the work sleep for the time the conversion needs. I think then my issue could also be solved by your PR.

Thanks for the details!

I agree that one way to handle these delays during the execution (e.g: not because of a bus transfer, but because the sensor needs it) is by scheduling work item to execute when the delay is over. This can be accomplished by expanding the RTIO Workqueue service here: https://github.com/ubieda/zephyr/blob/26970467243b50b3bc9f8ff30a0b6938a2579209/subsys/rtio/rtio_workq.c#L73.

I'd be happy to provide this feature expansion by following-up on a separate PR after #74435 it's merged.

EDIT: It seems the approach would not be as straightforward as I had suggested. However, seems like it should still fit naturally within what the RTIO Workqueue service would cover.

[FlorianWeber1018]

@ubieda could you please extend the RTIO Workqueue service so that it is possible to start delayed work as you have already mentioned?
Many thanks and kind regards,
Florian

[ubieda]

@ubieda could you please extend the RTIO Workqueue service so that it is possible to start delayed work as you have already mentioned?

Many thanks and kind regards,

Florian

I'll take a look soon and let you know. Thanks!

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[FlorianWeber1018]

@ubieda at the moment it looks like that delayed work in the rtio workq (#77100) will not be available in the near future. As you have mentioned in the issue, it is not as straight forward as you have thought at the beginning.
Should i proceed with this PR by using the system workq instead like the code is written now?

[ubieda]

@FlorianWeber1018 yeah - let's move forward without relying on it and refactor once the feature lands. Feel free to mark this PR as ready for review when it's finalized to get the process going.

[ubieda]

This is required for Big Endian arch's. Please discard change.

[FlorianWeber1018]

From my Perspective doing the conversion this way (buf[1] | (buf[2] << 8)) should already taken care about endianness because the shift operator works at the level of the logical value, not how it is ordered in memory.
<< shifts always to the the MSB (even across byte boarders) disregarding the endianness. << is also an arithmetic operation.

Endianness only affects how values are stored in memory (the order of bytes), but it doesn’t affect how the CPU performs bitwise operations on the value itself.
The CPU’s shift operation takes place on the entire bit pattern, treating it as a single number, not as a collection of bytes in memory.

Do you have a BE SoC to test it?
In fact that this change is not related to this PR I should revert it anyway and reach out a dedicated PR for that.

[ubieda]

From my Perspective doing the conversion this way (buf[1] | (buf[2] << 8)) should already taken care about endianness because the shift operator works at the level of the logical value, not how it is ordered in memory.
<< shifts always to the the MSB (even across byte boarders) disregarding the endianness. << is also an arithmetic operation.

Endianness only affects how values are stored in memory (the order of bytes), but it doesn’t affect how the CPU performs bitwise operations on the value itself.
The CPU’s shift operation takes place on the entire bit pattern, treating it as a single number, not as a collection of bytes in memory.

Do you have a BE SoC to test it?

Not sure I understand: the code is mapping an array of bytes to an int16_t based on your CPU endianness. Doing the same bit-shifting on Little-endian and Big-endian will give you different results.

[ubieda]

Update: I went and looked an old test-code I had for this use-case and ran it on BE and LE. Here's the results:

Source code:

#include <stdio.h>
#include <stdint.h>

int main(void)
{
    printf("Exercise: Converting little-endian value in big-endian system!\n");

{
    uint8_t array[] = {0x12, 0x34};
    uint16_t *val = (uint16_t *)array;

    uint16_t num = array[0] | (array[1] << 8);

    printf("Array interpreted as integer: 0x%04X = %d\n", *val, *val);
    printf("Integer after conversion: 0x%04X = %d\n", num, num);
}

    return 0;
}

Console output:

ubieda@ubieda-ubuntu:~/programming/c_programming/endianness/src$ mips-linux-gnu-gcc-10 main.c -o main_be.o && qemu-mips -L /usr/mips-linux-gnu/ ./main_be.o
Exercise: Converting little-endian value in big-endian system!
Array interpreted as integer: 0x1234 = 4660
Integer after conversion: 0x3412 = 13330
ubieda@ubieda-ubuntu:~/programming/c_programming/endianness/src$ gcc main.c -o main.o && ./main.o
Exercise: Converting little-endian value in big-endian system!
Array interpreted as integer: 0x3412 = 13330
Integer after conversion: 0x3412 = 13330

It appears this is indeed needed. I suggest putting it on a separate patch or creating another PR with proper description.