simplelink: wifi: avoid re-declaring pthread_self() #44

cfriedt · 2022-10-30T12:30:40Z

The normative spec says this returns pthread_t, not void *.

It's probably better to simply #include <pthread.h> outside of this file explicitly (due to current non-standard prefixed path in Zerphy) than to re-declare it with a potentially conflicting signature.

stephanosio · 2022-10-30T13:15:51Z

simplelink/source/ti/drivers/net/wifi/source/driver.h

-extern void * pthread_self(void);
+extern pthread_t pthread_self(void);


This will not work since this header does not include any headers that would provide the typedef for pthread_t.

On the contrary - it does work:

twister -i -p cc3220sf_launchxl -s samples/net/sockets/http_get/sample.net.sockets.http_get.offload.simplelink Renaming output directory to /Users/cfriedt/workspace/zephyrproject/zephyr/twister-out.21 INFO - Using Ninja.. INFO - Zephyr version: zephyr-v3.2.0-987-gad7ffc405d9a INFO - Using 'zephyr' toolchain. INFO - Building initial testsuite list... INFO - Writing JSON report /Users/cfriedt/workspace/zephyrproject/zephyr/twister-out/testplan.json INFO - JOBS: 8 INFO - Adding tasks to the queue... INFO - Added initial list of jobs to queue INFO - Total complete: 1/ 1 100% skipped: 0, failed: 0 INFO - 1 test scenarios (1 test instances) selected, 0 configurations skipped (0 by static filter, 0 at runtime). INFO - 1 of 1 test configurations passed (100.00%), 0 failed, 0 skipped with 0 warnings in 35.05 seconds INFO - In total 0 test cases were executed, 1 skipped on 1 out of total 510 platforms (0.20%) INFO - 0 test configurations executed on platforms, 1 test configurations were only built. INFO - Saving reports... INFO - Writing JSON report /Users/cfriedt/workspace/zephyrproject/zephyr/twister-out/twister.json INFO - Writing xunit report /Users/cfriedt/workspace/zephyrproject/zephyr/twister-out/twister.xml... INFO - Writing xunit report /Users/cfriedt/workspace/zephyrproject/zephyr/twister-out/twister_report.xml... INFO - Run completed

See also passing CI
zephyrproject-rtos/zephyr#51784

then that is purely by luck because some C files that include this header happen to include POSIX headers.

then that is purely by luck because some C files that include this header happen to include POSIX headers.

also that brings the question of, why is this pthread_self forward declaration necessary if the POSIX headers can be included in this scope?

also that brings the question of, why is this pthread_self forward declaration necessary if the POSIX headers can be included in this scope?

As you are aware, Zephyr POSIX headers cannot be portably included in this scope. That is the issue fixed by zephyrproject-rtos/zephyr#51771, which now indirectly depends on this PR.

Let's not make this a chicken-egg problem please.

until then, this header should be including zephyr/posix/pthread.h (assuming pthread.h is include-able in this scope).

in fact, I just removed this extern pthread_t pthread_self(void); entirely, and it works, meaning pthread.h is already included somewhere.

Looks like pthread.h is included here:

hal_ti/simplelink/source/ti/drivers/net/wifi/porting/cc_pal.h

Line 54 in 000b944

#include <zephyr/posix/pthread.h>

stephanosio · 2022-10-30T13:19:42Z

also why is this HAL using the pthread functions?

hal_ti/simplelink/source/ti/drivers/net/wifi/porting/user.h

Line 998 in 3da6fae

#define sl_LockObjLock(pLockObj,Timeout) pthread_mutex_lock(pLockObj)

A proper port ought to be using the native Zephyr APIs here.

cfriedt · 2022-10-30T13:21:52Z

also why is this HAL using the pthread functions?

hal_ti/simplelink/source/ti/drivers/net/wifi/porting/user.h

Line 998 in 3da6fae

#define sl_LockObjLock(pLockObj,Timeout) pthread_mutex_lock(pLockObj)

A proper port ought to be using the native Zephyr APIs here.

@stephanosio - that's a great question to ask TI. However, POSIX is not evil - it's actually a great interface to use for portable code to different operating systems (Portable Operating System Interface)

wrong assumptions used

stephanosio

the assumption is not wrong.

a header must include the headers that provides the dependencies it makes use of.

stephanosio · 2022-10-30T13:24:36Z

@stephanosio - that's a great question to ask TI

no, when adding support for this chipset, the porting layer should have been updated to use the Zephyr native API. In that sense, this port is incomplete.

cfriedt · 2022-10-30T13:25:19Z

@stephanosio - that is WELL beyond the scope of this change. Feel free to create an issue though.

stephanosio · 2022-10-30T13:26:22Z

@stephanosio - that is WELL beyond the scope of this change. Feel free to create an issue though.

agreed. just pointing out that there is something fishy about this port.

stephanosio · 2022-10-30T13:27:35Z

However, POSIX is not evil - it's actually a great interface to use for portable code to different operating systems (Portable Operating System Interface)

POSIX is not evil, but as long as Zephyr's native API is not POSIX, it will come with the added footprint, which is evil.

The normative spec says this returns `pthread_t`, not `void *`. It's probably better to simply `#include <pthread.h>` outside of this file explicitly (due to current non-standard prefixed path in Zerphy) than to re-declare it with a potentially conflicting signature. Signed-off-by: Chris Friedt <[email protected]>

cfriedt · 2022-10-30T23:54:51Z

@stephanosio - removed the conflicting declaration

outdated

cfriedt · 2022-10-31T16:15:49Z

@stephanosio - does this look OK now? Please ack if so.

stephanosio · 2022-10-31T16:24:53Z

@stephanosio - does this look OK now? Please ack if so.

yes, but can you please update the Zephyr-side PR so CI can run on it?

cfriedt · 2022-10-31T17:50:48Z

@stephanosio - does this look OK now? Please ack if so.

yes, but can you please update the Zephyr-side PR so CI can run on it?

There isn't anything to update until this PR is merged.

stephanosio · 2022-10-31T17:53:57Z

There isn't anything to update until this PR is merged.

did zephyrproject-rtos/zephyr#51784 run with the latest iteration of this PR? I just want to make sure that nothing else broke because of the latest change.

cfriedt · 2022-10-31T19:41:02Z

@stephanosio I just force-pushed again to be sure

cfriedt requested a review from galak October 30, 2022 12:30

cfriedt self-assigned this Oct 30, 2022

cfriedt mentioned this pull request Oct 30, 2022

west.yml: bump hal_ti to not re-declare pthread_self() zephyrproject-rtos/zephyr#51784

Merged

stephanosio previously requested changes Oct 30, 2022

View reviewed changes

cfriedt requested a review from stephanosio October 30, 2022 13:26

cfriedt force-pushed the use-correct-pthread-self-signature branch from ea84987 to 5802cd2 Compare October 30, 2022 23:31

cfriedt force-pushed the use-correct-pthread-self-signature branch from 5802cd2 to 34ef193 Compare October 30, 2022 23:53

cfriedt changed the title ~~simplelink: wifi: use correct signature for pthread_self()~~ simplelink: wifi: avoid re-declaring pthread_self() Oct 30, 2022

stephanosio approved these changes Oct 31, 2022

View reviewed changes

cfriedt merged commit a5ffc6d into zephyrproject-rtos:master Oct 31, 2022

cfriedt deleted the use-correct-pthread-self-signature branch October 31, 2022 21:45

cfriedt mentioned this pull request Mar 19, 2023

CC13x2x7 and CC26X2X7 support #45

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

simplelink: wifi: avoid re-declaring pthread_self() #44

simplelink: wifi: avoid re-declaring pthread_self() #44

cfriedt commented Oct 30, 2022 •

edited

Loading

stephanosio Oct 30, 2022

cfriedt Oct 30, 2022

stephanosio Oct 30, 2022

stephanosio Oct 30, 2022

cfriedt Oct 30, 2022

stephanosio Oct 30, 2022 •

edited

Loading

stephanosio Oct 30, 2022 •

edited

Loading

stephanosio Oct 30, 2022

stephanosio commented Oct 30, 2022 •

edited

Loading

cfriedt commented Oct 30, 2022 •

edited

Loading

stephanosio left a comment

stephanosio commented Oct 30, 2022

cfriedt commented Oct 30, 2022 •

edited

Loading

stephanosio commented Oct 30, 2022

stephanosio commented Oct 30, 2022

cfriedt commented Oct 30, 2022

cfriedt commented Oct 31, 2022

stephanosio commented Oct 31, 2022

cfriedt commented Oct 31, 2022

stephanosio commented Oct 31, 2022

cfriedt commented Oct 31, 2022

		extern void * pthread_self(void);
		extern pthread_t pthread_self(void);

simplelink: wifi: avoid re-declaring pthread_self() #44

simplelink: wifi: avoid re-declaring pthread_self() #44

Conversation

cfriedt commented Oct 30, 2022 • edited Loading

stephanosio Oct 30, 2022

Choose a reason for hiding this comment

cfriedt Oct 30, 2022

Choose a reason for hiding this comment

stephanosio Oct 30, 2022

Choose a reason for hiding this comment

stephanosio Oct 30, 2022

Choose a reason for hiding this comment

cfriedt Oct 30, 2022

Choose a reason for hiding this comment

stephanosio Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

stephanosio Oct 30, 2022 • edited Loading

Choose a reason for hiding this comment

stephanosio Oct 30, 2022

Choose a reason for hiding this comment

stephanosio commented Oct 30, 2022 • edited Loading

cfriedt commented Oct 30, 2022 • edited Loading

stephanosio left a comment

Choose a reason for hiding this comment

stephanosio commented Oct 30, 2022

cfriedt commented Oct 30, 2022 • edited Loading

stephanosio commented Oct 30, 2022

stephanosio commented Oct 30, 2022

cfriedt commented Oct 30, 2022

cfriedt commented Oct 31, 2022

stephanosio commented Oct 31, 2022

cfriedt commented Oct 31, 2022

stephanosio commented Oct 31, 2022

cfriedt commented Oct 31, 2022

cfriedt commented Oct 30, 2022 •

edited

Loading

stephanosio Oct 30, 2022 •

edited

Loading

stephanosio Oct 30, 2022 •

edited

Loading

stephanosio commented Oct 30, 2022 •

edited

Loading

cfriedt commented Oct 30, 2022 •

edited

Loading

cfriedt commented Oct 30, 2022 •

edited

Loading