core: resolve circular dependency

[maribu]

Contribution description

Dependency graph before:

     +-------------------------------------+
     |                                     |
     |                                     V
+----------+        +-------+         +---------+
| thread.h | -----> | msg.h | ------> | sched.h |
+----------+        +-------+         +---------+
     ^                                     |
     |                                     |
     +-------------------------------------+

Dependency graph after:

                 +--------------+
                 | core_types.h |
                 +--------------+
                    ^   ^   ^
                    |   |   |
      +-------------+   |   +--------------+
      |                 |                  |
+----------+            |             +---------+
| thread.h |            |             | sched.h |
+----------+            |             +---------+
                        |                  ^
                    +-------+              |
                    | msg.h |--------------+
                    +-------+

Testing procedure

This PR doesn't change code. It only relocates it. Hence, binaries shouldn't change.

Issues/PRs references

This circular dependency became an issue in #16174

[benpicco]

Nice cleanup!

[kaspar030]

Pleas don't merge too quick, I'd like to take a closer look.

[benpicco]

Yea Murdock has found some isssues.

[kaspar030]

I'm not in favour of shuffling the headers around like this.

How about just forward declaring msg_t? The only reason thread.h needs msg_t is a single pointer. -> #16477

[aabadie]

This seems cleaner than #16477 (and needs a rebase).

[maribu]

I'm not in favour of shuffling the headers around like this.

How about just forward declaring msg_t? The only reason thread.h needs msg_t is a single pointer. -> #16477

That doesn't solve the issue that sched.h depends on thread.h depends on sched.h ...

[maribu]

@chrysn For this to be compatible with rust, riot_sys should use thread_t instead of struct _thread, right? That should also work with both current master and this PR.

[chrysn]

Rust has no difference between type names and named structs, and it looks at all the headers at once, so I don't think it makes any difference. Bindings already look like this:

#[repr(C)]
pub struct _thread {
    pub sp: *mut c_char,
    pub status: thread_status_t,
    // ...
}
pub type thread_t = _thread;

where thread_t is purely an alias for _thread and usable everywhere interchangeabl. That is, functions may declare either in their signature, but a user would use thread_t everywhere and it would be accepted -- it could just as well say pub struct thread_t { ... } / pub type _thread = thread_t;.

[maribu]

The issue is that with this PR, struct _thread is dropped and only thread_t is provided. It is a pity that Rust internally references the internal alias _thread. Would it be possible to drop this, so that this rust example would still build with this PR?

[chrysn]

The Rust bindings (of riot-sys) are always automatically regenerated from the used RIOT code. When `struct _thread` goes away, there will not be a `_thread` any more in riot-sys. Aaah I see -- riot-wrappers uses `_thread`. That's unfortunate, it shouldn't have been doing this. (In general, riot-sys contains a lot of symbols that one should rather not use -- basically everything there is no RIOT documentation for. That's the nature of going from an unnamespaced language to a namespaced one). Fix coming, sorry for both the bug and me not getting it originally.

[chrysn]

Please rebase onto (I prefer merging in, but was told GitHub doesn't handle this workflow) #17514 once that's merged. (Let's hope there are a few fast ones in the CI queue). [edit: You can also just `cargo update` locally in the failing test to verify it passes now]

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[miri64]

Huh, what happened here? Seems somewhat important!

[maribu]

I still think this should go in

[maribu]

Rebased and reordered the commits so that the addition of missing #includes (which previously were pulled in indirectly) are added first, and the actual change comes afterwards. That way bisecting should be easier.

[Teufelchen1]

Did we have another issue with circular dependency since #16174?
I feel a bit like not having enough insights into /core/ to have an opinion but if taken with a grain of salt, this looks like a reasonable change. I would approve it :>
Maybe you can rebase if you are still interested maribu?

[maribu]

We eventually have worked around the issue. But generally, circular dependencies in C headers are a bit messy, so this may be useful later down the road.

Maybe close this for now and pick this up again when someone bumps their feet on this again?

[Teufelchen1]

Closed.

• Problem has been identified
• Multiple proposed solution, including this one, didn't get "enough approval"
• Solution is not urgently needed
• Will be revisited if someone finds another circular dependency error / issue

Thanks for your proposal @maribu! :)