-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Should we care about ABI and semantic versioning for stlink libs? #1075
Comments
Can you explain this issue in more detail and give some examples? |
This is how struct stlink_chipid_params look like in version 1.6.0 struct stlink_chipid_params {
uint32_t chip_id;
char *description;
enum stlink_flash_type flash_type;
uint32_t flash_size_reg;
uint32_t flash_pagesize;
uint32_t sram_size;
uint32_t bootrom_base;
uint32_t bootrom_size;
}; This is how it looks now : struct stlink_chipid_params {
uint32_t chip_id;
char *description;
enum stlink_flash_type flash_type;
bool has_dual_bank;
uint32_t flash_size_reg;
uint32_t flash_pagesize;
uint32_t sram_size;
uint32_t bootrom_base;
uint32_t bootrom_size;
uint32_t option_base;
uint32_t option_size;
}; As you can see there is bool has_dual_bank was added before flash_size_reg field. |
But, actually, I didn't dig deeper. So it is a question that we haven't similar breakage for previous releases. So there is a question should we care about it in future? |
I see what you mean. I believe this is also what you meant with the preference to allocate new parameters at the end of structures. |
Maybe this link is somehow useful here: I think the intermediate structure may help. |
Yes, I have ABI in my mind when saying that. But this trick works only if in user code used pointers to structs. At first sight, it correct for our codebase. |
@chenguokai Looks useful indeed. |
@slyshykO Can you check the codebase on this? |
I will think about it. |
Well, I'd go through such spots where this topic could be relevant. |
I think, for the best, external programs should not use the structure directly. Needs to revise the contents of the include files. External applications should only be given the stlink_* functions and enums (encapsulation principle). Also I think (and want) to rewrite the common.c. It has become very large and difficult to understand and support. I think that it should be divided into several files corresponding to the type of flash memory (STLINK_FLASH_TYPE_ *). |
@Nightwalker-87 I have no spare time to work on this project now. About this issue. I think we can close it now. But remember that ABI is important, and try not to break it by new commits. |
@slyshykO Thx for the response.
As far as I understood from the context, it is not unified yet and/or has not been verified either. |
@Ant-ON Can you verify if this is implemented consistently throughout the whole code base? |
@Nightwalker-87 there have been no such changes since the 1.7.0 release |
@Ant-ON I was referring to the general status quo, not in reference to a certain release. |
@Nightwalker-87 It is necessary to follow the PR and see that the fields are not add in the middle of the structure. Also we can write (add reference to this issue) about this in the development documentation. |
Ok, we should do this then to finally address this issue. Are there any inconsistencies currently present that have not been taken note of yet? |
@slyshykO Can you recheck the codebase for any leftovers by now? I'll add a respective note to the documentation soon. |
Hi @Nightwalker-87 . I am thinking about this issue. After that, we will freeze the sizes of the public structs. By the way, we also should hide as many structs as we can. So I propose to add milestone for this purpose and do steps toward the target. Cause it is impossible to do in one commit. |
I did a quick look at the codebase and spot only one struct size change since 1.7.0. |
Ok, let's proceed as follows:
I'd not open a separate milestone for this but use the 1.8.0 milestone for this instead. |
In my opinion, the other branch is a non-working option. Cause it is big refactoring. And it is impossible to have 2 synchronized branches with such a big diff. |
Well, to me it does not sound like a topic to push into v1.7.1 then, which should rather remain a bugfix and minor feature release. |
Seen from a more general perspective it would be favourable to resolve the oldest open bugfix topics in 1.7.x first to avoid dragging these along while the codebase is undergoing mayor rework. Fortunately their amount is constantly decreasing and I hope most of them can be solved within the 1.7.1 release. |
@slyshykO Can you address #1075 (comment) then and take a closer look into the codebase as well? |
Without refactoring our codebase, we should mark in the readme that we break API every release, even minor ones. |
This issue is now closed due to inactivity. |
I see that at least 2 structures that expose over lib border changed its shape between ver 1.6.0 and 1.6.1 and still changes.
It is struct stlink_t and struct stlink_chipid_params.
So if we care about ABI we should change their shape in backwards-compatible form.
The text was updated successfully, but these errors were encountered: