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(pthread): Add esp_pthread function implementations for linux target (IDFGH-13485) #14384

Conversation

cristianfunes79
Copy link
Contributor

@cristianfunes79 cristianfunes79 commented Aug 16, 2024

This PR is to add functionality to pthread port for linux target. It extends changes done in #14339

DESCRIPTION

As soon as pthread implementation for linux uses libpthread, it is not possible to use esp custom functions for setting pthread parameters.
But this implementation at least can be use to validate arguments passed to the functions.

DETAILED DESCRIPTION

When using pthread_create esp implementation, it uses freeRTOS task behind to create a new execution thread. This implementation uses pthread_getspecifics to get values set by esp_pthread_set_cfg (const esp_pthread_cfg_t *cfg) (see https://github.com/espressif/esp-idf/blob/master/components/pthread/pthread.c#L309).
When using linux target, that custom pthread implementation is no more used, so all esp_pthread.h functions doesn't affect thread creation.

Copy link

github-actions bot commented Aug 16, 2024

Messages
📖 🎉 Good Job! All checks are passing!

👋 Hello cristianfunes79, we appreciate your contribution to this project!


📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...


This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...


We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against 92bfb26

@espressif-bot espressif-bot added the Status: Opened Issue is new label Aug 16, 2024
@github-actions github-actions bot changed the title fix(pthread): Add esp_pthread function implementations for linux target fix(pthread): Add esp_pthread function implementations for linux target (IDFGH-13485) Aug 16, 2024
@cristianfunes79
Copy link
Contributor Author

Closes #14387

@ESP-Marius ESP-Marius requested a review from 0xjakob August 19, 2024 03:07
@ESP-Marius
Copy link
Collaborator

Thanks for adding this, we'll take a look!

Copy link
Contributor

@0xjakob 0xjakob left a comment

Choose a reason for hiding this comment

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

@cristianfunes79 Thanks for this PR, it looks great. I just have one minor comment.

I think this is a good improvement as it keeps the basic behavior of the API on the Linux target aligned with the behavior of the API on the chip targets. This means less surprises for users.

}
*p = *cfg;
p->stack_alloc_caps = heap_caps;
pthread_setspecific(s_pthread_cfg_key, p);
Copy link
Contributor

Choose a reason for hiding this comment

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

Really minor thing: I'd suggest to add an assert here to make sure pthread_setspecific() succeeds, even though the chance is probably very little:

Suggested change
pthread_setspecific(s_pthread_cfg_key, p);
int __attribute((unused)) res = pthread_setspecific(s_pthread_cfg_key, p);
assert(res == 0);

You may need to #include assert.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've tried it but it is not so straight forward, as soon as linux doesnt call esp_pthread_init, cfg_key is never created and setspecific assertion fails. I'll take a look on how to resolve that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xjakob Actually I'm thinking about all functions marked as ESP_SYSTEM_INIT_FN. Does it make sense to create a ticket to implement a way to run such functions in linux?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've tried it but it is not so straight forward

Oh, sorry, I didn't think about that. I think you could try to use __attribute__((constructor)) to create a constructor function to initialize the key. __attribute__((constructor)) is documented in the GCC function attributes doc page

ESP_SYSTEM_INIT_FN won't work on Linux. It's not clear yet, either, if we will implement it on Linux or not. We have a ticket for exactly this in our own ticket system already. Unless you have a concrete reason to implement it, there is no need to create a ticket here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll add constructor. The reason I can think to add ESP_SYSTEM_INIT_FN is to have more similar firmware behavior in linux than in xtensa. Maybe it could be as simple as creating a #define ESP_SYSTEM_INIT_FN(foo) __attribute__((constructor)) foo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xjakob this is ready for review

@cristianfunes79 cristianfunes79 force-pushed the add_pthread_functionality_cxx branch from 7dcfd0b to 92bfb26 Compare August 22, 2024 15:25
@0xjakob
Copy link
Contributor

0xjakob commented Sep 3, 2024

sha=92bfb26639e40149d02140a1f672254c4e161f15

@0xjakob 0xjakob added the PR-Sync-Merge Pull request sync as merge commit label Sep 3, 2024
@0xjakob
Copy link
Contributor

0xjakob commented Sep 3, 2024

@cristianfunes79 We're merging this into our internal repository. Afterwards, it should soon be synchronized to master.

@espressif-bot espressif-bot added Status: Reviewing Issue is being reviewed and removed Status: Opened Issue is new labels Sep 6, 2024
@espressif-bot espressif-bot assigned ESP-Marius and unassigned 0xjakob Sep 14, 2024
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Reviewing Issue is being reviewed labels Sep 19, 2024
@Alvin1Zhang
Copy link
Collaborator

Thanks for contribution again, changes have been merged with 4c7d9f9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants