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

apis: organize into folders #546

Merged
merged 1 commit into from
Jun 12, 2024
Merged

apis: organize into folders #546

merged 1 commit into from
Jun 12, 2024

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Jun 7, 2024

This is an annoying PR, so fair to close this.

However, one thing we learned after implementing all of the libtock-c drivers is that a flat folder structure gets very unwieldy, and having folders to organize is useful. So, it might be a good idea to apply that here as well.

I used the same folder names as libtock-c.

@alevy
Copy link
Member

alevy commented Jun 11, 2024

I'm planning to approve this (it needs a rebase/merge anyway), but I wonder if this is not an opportunity to rethink how to structure responsibilities in libtock-rs.

In particular, I'd advocate we avoid (or reverse) centralizing all of the functionality any userspace process could possibly want into this library, which it currently does.

Currently libtock-rs imports and re-exports basically everything in apis/, I think this fails to take advantage Cargo's package management and forces this repo/library to be the arbiter of what the "Right" driver is for every kind of functionality.

Anyway, if I'm not downvoted to oblivion, I should probably turn this into an issue and not block this PR (I think this kind of organization makes sense even if/when these crates are in different repos, or otherwise independent of libtock-rs "core").

@bradjc
Copy link
Contributor Author

bradjc commented Jun 11, 2024

On the other hand, I think we should avoid making individual app developers responsible for deciding (and figuring out) what the "Right" driver is for every kind of functionality they may need.

In general I agree that we can use cargo to effectively split the core syscall mechanism from driver APIs and implementations. However, I think we should be able to name and talk about groups of apis. If we can give them names then we can probably teach others how to use them. For example, something like "the arduino interface" is a group of APIs that have a name.

@jrvanwhy jrvanwhy added the significant Indicates a PR is significant as defined by the code review policy. label Jun 11, 2024
@jrvanwhy
Copy link
Collaborator

Currently libtock-rs imports and re-exports basically everything in apis/, I think this fails to take advantage Cargo's package management and forces this repo/library to be the arbiter of what the "Right" driver is for every kind of functionality.

Are you referring to libtock in particular? There's nothing that requires users of libtock-rs to depend on libtock specifically -- users who are particular about what is included in their library should just depend on the crates they want.

Or are you saying that the various drivers in apis/ don't belong in the libtock-rs repository?

jrvanwhy
jrvanwhy previously approved these changes Jun 11, 2024
@alevy alevy added this pull request to the merge queue Jun 12, 2024
Merged via the queue into master with commit fd2f66d Jun 12, 2024
3 checks passed
@alevy alevy deleted the api-folders branch June 12, 2024 13:17
@alevy
Copy link
Member

alevy commented Jun 12, 2024

Are you referring to libtock in particular? There's nothing that requires users of libtock-rs to depend on libtock specifically -- users who are particular about what is included in their library should just depend on the crates they want.

OK, meaning platform and runtime? And libtock is kind of morally like Rust's std? I think that's fine. I'm not sure I agree with the defaults here-.. but that's more aesthetic. Examples doing this would be nice. But...

Or are you saying that the various drivers in apis/ don't belong in the libtock-rs repository?

Basically yes. If libtock is going to include a bunch of the "Right" interfaces to use, we should be much more choosy about which specific interfaces are included there. For example, I think no version of SPI or I2C drivers belong there, anything embedded-hal probably doesn't belong there (at least not if and until that stack becomes what we view as the default), and likely some other api crates are insufficiently mature or good yet to include.

I'm a bit less concerned about those crates being in this repo, though them being there does seem to discourage development of non-canonical crates out-of-tree.

I suggest we table this to either a call or TockWorld (or a separate issue/e-mail thread), though. This is really something that came up for me due to reviewing this PR, but isn't actually directly relevant to it.

@jrvanwhy
Copy link
Collaborator

Are you referring to libtock in particular? There's nothing that requires users of libtock-rs to depend on libtock specifically -- users who are particular about what is included in their library should just depend on the crates they want.

OK, meaning platform and runtime?

Yes. For example, an app that wants to minimize binary size at the expense of debugging functionality should depend on libtock_platform, libtock_runtime, and libtock_small_panic instead of depending on libtock (because libtock brings in libtock_debug_panic).

And libtock is kind of morally like Rust's std? I think that's fine. I'm not sure I agree with the defaults here-.. but that's more aesthetic. Examples doing this would be nice. But...

It's more akin to futures, which just re-exports things from other crates in its repository.

And yes, I agree that examples would be nice. The demos/ directory gives us a place to do that (because anything in examples/ automatically depends on libtock).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
significant Indicates a PR is significant as defined by the code review policy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants