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

Fix #4911 : Partially rework some code to remove warnings about potential non-aligned memory accesses #4912

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

TheMalkavien
Copy link
Contributor

@TheMalkavien TheMalkavien commented Sep 30, 2024

Hi,

As discussed in the issue #4911, here is my approach to contain the memory alignment issue

There is some very easy fixes based on memcpy insted of pointer arithmetic and dereferrence. This is a quick win.

But I had to think of another way to work with the radiobuffer bytebuffer, because of how it was previously handled, the only way was using pointer arithmetic to access payload after the header.

So I just replaced the byte buffer by a struct containing header then payload :

/**
 * This structure represent the structured buffer : a PacketHeader then the payload. The whole is
 * MAX_LORA_PAYLOAD_LEN + 1 length
 * It makes the use of its data easier, and avoids manipulating pointers (and potential non aligned accesses)
 */
typedef struct {
    /** The header, as defined just before */
    PacketHeader header;

    /** The payload, of maximum length minus the header, aligned just to be sure */
    uint8_t payload[MAX_LORA_PAYLOAD_LEN + 1 - sizeof(PacketHeader)] __attribute__ ((__aligned__));

} RadioBuffer;

As you can see, this is very simple. And I replaced all the pointer code by struct accesses :

PacketHeader *h = (PacketHeader *)radiobuf;
h->from

becomes :
radioBuffer.header.from

and
radiobuf + sizeof(PacketHeader)
becomes :
radioBuffer.payload

Its more elegant too :)

The result is, less memory-aligned warnings, and a more safe code to use, and depending of your coding style, more readable I guess.
I quickly tested it on RP2040, HTv3 and HT Tracker 1.1 (to test the gps part) without seeing any issue.

This should Fix #4911 (partially at least) and gives a good start (or just ideas) to a more robust code.

Regards,

Copy link
Member

@caveman99 caveman99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We need to make sure the packetheader stays the same size and alignment no matter what processor architecture we're running on.

@thebentern thebentern merged commit 553514e into meshtastic:master Sep 30, 2024
104 of 105 checks passed
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

Successfully merging this pull request may close these issues.

[Bug]: More potential non aligned memory accesses (focused on RP2040 but not only)
3 participants