Skip to content

Conversation

taunusflieger
Copy link

This PR addresses the differences (some fields are too small, one field is too large) in field sizes described in issue esp-rs/esp-idf-svc#204. It ensures, that the fields in FirmwareInfo are large enough to hold the fields defined in esp_app_desc_t. For details see the issue above.

@MabezDev
Copy link
Member

MabezDev commented Jan 3, 2023

embedded-svc is meant to be completely unaware of esp-idf, so I don't think we should be changing this struct to fit esp-idf's alignment needs unless there is another valid reason to do so. Instead, I think that esp-idf-svc should be handling the conversion from the esp-idf type, to the svc type.

If it turns out the embedded-svc struct is unfit for its purpose, we can change it. Thoughts @ivmarkov @zRedShift?

@ivmarkov
Copy link
Collaborator

embedded-svc is meant to be completely unaware of esp-idf, so I don't think we should be changing this struct to fit esp-idf's alignment needs unless there is another valid reason to do so. Instead, I think that esp-idf-svc should be handling the conversion from the esp-idf type, to the svc type.

If it turns out the embedded-svc struct is unfit for its purpose, we can change it. Thoughts @ivmarkov @zRedShift?

I agree. If there is some field in the embedded-svc struct which is too short, let's extend it, but 100% alignment is not what we are seeking out here.

@4rzael
Copy link

4rzael commented Jun 2, 2025

The current implementation where the version field is 24 bytes long, leads to a bug in esp-idf-svc, where EspOta::get_running_slot fails when the partition's version is too long.

This can happen for example when using git tags with fairly long names (because esp-idf's version is based on git describe.

I'm not sure if this should be solved here, by increasing field sizes, or in esp-idf-svc, by truncating the fields if needed ?

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.

4 participants