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

Introduce Pigweed Module #53760

Closed
wants to merge 5 commits into from
Closed

Conversation

rbbrns
Copy link
Contributor

@rbbrns rbbrns commented Jan 12, 2023

No description provided.

@zephyrbot zephyrbot added manifest manifest-pigweed DNM This PR should not be merged (Do Not Merge) labels Jan 12, 2023
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jan 12, 2023

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
pigweed N/A google/pigweed@4f10c43 N/A

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@carlescufi
Copy link
Member

Hi, please follow the process described here:
https://docs.zephyrproject.org/latest/contribute/external.html

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

As I said multiple times, why do we need to keep adding modules in the repository that only a few applications will use? When using the example-application model, applications can add any module as needed, so I don't see the point of growing module list. We already carry ~6-7Gb of modules, so after following getting started guide one will end up with less disk space than, e.g. if cloning Linux Kernel with all its history.

edit: Also, it looks like Pigweed overlaps/clashes with some functionality offered by Zephyr.

@pdgendt
Copy link
Collaborator

pdgendt commented Jan 13, 2023

As I said multiple times, why do we need to keep adding modules in the repository that only a few applications will use? When using the example-application model, applications can add any module as needed, so I don't see the point of growing module list. We already carry ~6-7Gb of modules, so after following getting started guide one will end up with less disk space than, e.g. if cloning Linux Kernel with all its history.

I somewhat agree here, however there are pros to introduce a module inside zephyr.

  • Based on maintainer commitment, keep up to date with latest zephyr
  • Adhere to the zephyr guidelines and reviewing process
  • Reach a bigger audience, a separate project probably won't get noticed as easily

The getting started guide does indeed download/use a lot of data, maybe we could improve with some opt-in system instead of opt-out (download on demand with Kconfig?).

For pigweed specifically, is this related to Matter support?

@gmarull
Copy link
Member

gmarull commented Jan 13, 2023

  • Based on maintainer commitment, keep up to date with latest zephyr

That's true, but at the cost of Zephyr (CI time, non-module developers adjusting for changes, etc.).

  • Adhere to the zephyr guidelines and reviewing process

Why do we need to impose Zephyr guidelines to external code? It's really up to the module on what to do: coding style, languages, CI, review process, etc.

  • Reach a bigger audience, a separate project probably won't get noticed as easily

This can be solved if we have a place to promote/list modules. But why does Zephyr need to care about module promotion? Also, stuff like Pigweed or LVGL are already quite popular. They can offer Zephyr integration as a plus. Actually, I think Zephyr would benefit more from modules promoting Zephyr support other than us claiming integration.

A sad example, but partially true since lvgl is now unmaintained in Zephyr and upstream likely doesn't consider Zephyr as a priority.
https://docs.lvgl.io/master/get-started/os/nuttx.html
https://docs.lvgl.io/master/get-started/os/zephyr.html

@yperess
Copy link
Collaborator

yperess commented Jan 17, 2023

Jumping in on this thread, that's on me that I didn't create the RFC quickly enough. I'm sorry. Pigweed should help Zephyr quite a bit by adding several modules that would otherwise be whole subsystems in Zephyr. These modules are aimed at embedded development and will cost the Zephyr community very little to maintain since the Pigweed team has offered to own the integration. You can run a search on Discord and see several mentions around Pigweed and several more around RPCs which Pigweed would provide us for free. The most recent example is the discussion around https://discord.com/channels/720317445772017664/733037199251079168/1017199830474440805

@gmarull
Copy link
Member

gmarull commented Jan 17, 2023

Jumping in on this thread, that's on me that I didn't create the RFC quickly enough. I'm sorry. Pigweed should help Zephyr quite a bit by adding several modules that would otherwise be whole subsystems in Zephyr. These modules are aimed at embedded development and will cost the Zephyr community very little to maintain since the Pigweed team has offered to own the integration. You can run a search on Discord and see several mentions around Pigweed and several more around RPCs which Pigweed would provide us for free. The most recent example is the discussion around discord.com/channels/720317445772017664/733037199251079168/1017199830474440805

This all sounds to me application-specific requirements, so I still don't understand the benefit of pulling something by default (discoverability has other solutions, IMO). Applications that need pigweed can simply do this in their west.yaml:

manifest:
  remotes:
    - name: pigweed
      url-base: https://pigweed.googlesource.com/pigweed
  projects:
    - name: pigweed
      remote: pigweed
      revision: main
      path: modules/pigweed

No need to involve upstream Zephyr.

@rbbrns rbbrns force-pushed the pigweed branch 2 times, most recently from 32a1931 to 2efa089 Compare January 17, 2023 17:34
This installs the pigweed python module in the twister workflow.
This is just for testing, should not be submitted.
This pigweed python module will be part of requirements.txt if this
module is added.
Add initial rules for the pigweed module. This adds pigweed to west
and adds the required pigweed python dependency.

Samples and documentation will be added in follow-on commits.

Signed-off-by: Rob Barnes <robbarnes@google.com>
Add a sample of the pigweed module pw_tokenizer that encodes and
decodes a simple string.

The sample was originally published at https://pigweed.googlesource.com/pigweed/zephyr-integration/+/refs/heads/main/samples/pw_tokenizer/

The sample can be built via:

```
west build -p=always -b native_posix samples/modules/pigweed/pw_tokenizer
```

Signed-off-by: Rob Barnes <robbarnes@google.com>
Add a sample application for the pigweed pw_rpc module.

This sample was orginally published at https://pigweed.googlesource.com/pigweed/zephyr-integration/+/refs/heads/main/samples/pw_rpc

The sample can be built via:

```
west build -p=always -b native_posix samples/modules/pigweed/pw_rpc
```

Signed-off-by: Rob Barnes <robbarnes@google.com>
Add a readme for the pigweed module. Includes an overview of the module,
requirements, and building instructions. It also covers maintenance of the
module.

The samples can be tested via:

```
./scripts/twister -c -T samples/modules/pigweed
```

Signed-off-by: Rob Barnes <robbarnes@google.com>
@yperess
Copy link
Collaborator

yperess commented Jan 19, 2023

Jumping in on this thread, that's on me that I didn't create the RFC quickly enough. I'm sorry. Pigweed should help Zephyr quite a bit by adding several modules that would otherwise be whole subsystems in Zephyr. These modules are aimed at embedded development and will cost the Zephyr community very little to maintain since the Pigweed team has offered to own the integration. You can run a search on Discord and see several mentions around Pigweed and several more around RPCs which Pigweed would provide us for free. The most recent example is the discussion around discord.com/channels/720317445772017664/733037199251079168/1017199830474440805

This all sounds to me application-specific requirements, so I still don't understand the benefit of pulling something by default (discoverability has other solutions, IMO). Applications that need pigweed can simply do this in their west.yaml:

manifest:
  remotes:
    - name: pigweed
      url-base: https://pigweed.googlesource.com/pigweed
  projects:
    - name: pigweed
      remote: pigweed
      revision: main
      path: modules/pigweed

No need to involve upstream Zephyr.

Correct, that's what we currently do. The main reason to involve upstream Zephyr is to make the Pigweed module more discoverable and easily accessible. The Pigweed team is currently working on a slide deck to highlight the benefits they would bring the Zephyr community along with a few of our own success stories of Zephyr + Pigweed. I personally think it would provide new projects a lot of benefit by filling in some gaps that we currently have. Basically, my concern would be that we would end up replicating some of Pigweed's modules as subsystems which is just a waste of time and effort when we can get them for free.

@cfriedt
Copy link
Member

cfriedt commented Jan 23, 2023

As I said multiple times, why do we need to keep adding modules in the repository that only a few applications will use?

Posting a link to my response of this same critique here #54013 (comment) .

TL; DR - value is in the eye of the beholder. ModuleX may have little value for Bob, but it could have great value for Alice. Few applications use ModuleX until more applications use ModuleX. All modules are optional.

Additionally, if it's CI time that is a concern, then we should really devise a better scheme for CI.

@mgielda
Copy link

mgielda commented Jan 31, 2023

+1 on @cfriedt's comment. IMO we should be building an ecosystem where (maintained) optional external modules are welcome and not discouraged, and if our workflow prevents that, we should discuss and try to improve our workflow. What we are communicating outwards from a marketing perspective is that Zephyr is an open ecosystem targeting many use cases, so it is inevitable that we will have modules that may not matter from the perspective of use case X, but may be critical for people doing Y, and for me that is just a normal sign of Zephyr's growth.

@mgielda
Copy link

mgielda commented Jan 31, 2023

By the way, I would really really really love that we solve the "the recommended way to do anything is to pull everything" problem at the core by having some sort of lazy-load style opt-in package/toolchain management.

For me this would be the number one west feature I miss - detecting what you need based on what you are trying to do and offering to just download the necessary modules / toolchains (rather than just expecting that, for toolchains, you get the entire SDK, and, for modules, getting them all en masse). It's not that nobody ever did this - PlatformIO has this kind of functionality figured out neatly. This would help us greatly simplify our getting started guide (of course we could have an "extended setup guide" if you do want to set everything up manually or get everything).

Generally speaking our default Zephyr "to build a hello world, please pull the entire world" strategy is very problematic for drive-by contributors, new and sporadic users who don't have Zephyr set up on all of the machines they use (which includes myself), any hands-on tutorials and demos, quick presentations and proof-of-concepts, and CI-vs-interactive-workflow reuse, and it is a widely discussed concern (at least it popped up a lot at ZDS, as well as in discussions with silicon vendors and developer surveys etc.)

If we find ourselves saying "let's not pull another module since we already force people to download 6-7 gigs by default" then we should be asking ourselves the question "why are we doing that".

@gmarull
Copy link
Member

gmarull commented Jan 31, 2023

By the way, I would really really really love that we solve the "the recommended way to do anything is to pull everything" problem at the core by having some sort of lazy-load style opt-in package/toolchain management.

For me this would be the number one west feature I miss - detecting what you need based on what you are trying to do and offering to just download the necessary modules / toolchains (rather than just expecting that, for toolchains, you get the entire SDK, and, for modules, getting them all en masse). It's not that nobody ever did this - PlatformIO has this kind of functionality figured out neatly. This would help us greatly simplify our getting started guide (of course we could have an "extended setup guide" if you do want to set everything up manually or get everything).

Generally speaking our default Zephyr "to build a hello world, please pull the entire world" strategy is very problematic for drive-by contributors, new and sporadic users who don't have Zephyr set up on all of the machines they use (which includes myself), any hands-on tutorials and demos, quick presentations and proof-of-concepts, and CI-vs-interactive-workflow reuse, and it is a widely discussed concern (at least it popped up a lot at ZDS, as well as in discussions with silicon vendors and developer surveys etc.)

If we find ourselves saying "let's not pull another module since we already force people to download 6-7 gigs by default" then we should be asking ourselves the question "why are we doing that".

+1, agreed

@cfriedt cfriedt marked this pull request as ready for review January 31, 2023 10:53
@zephyrbot zephyrbot requested a review from stephanosio January 31, 2023 10:54
@cfriedt cfriedt marked this pull request as draft January 31, 2023 10:54
@fabiobaltieri
Copy link
Member

For me this would be the number one west feature I miss - detecting what you need based on what you are trying to do and offering to just download the necessary modules / toolchains (rather than just expecting that, for toolchains, you get the entire SDK, and, for modules, getting them all en masse). It's not that nobody ever did this - PlatformIO has this kind of functionality figured out neatly. This would help us greatly simplify our getting started guide (of course we could have an "extended setup guide" if you do want to set everything up manually or get everything).

RIOT does that, though it download in the build directory which is odd, they have a concept of package (https://github.com/RIOT-OS/RIOT/blob/master/pkg/nrfx/Makefile). Repo has the concept of groups (https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:manifest/full.xml;l=129-131) but that requires manual dependency maintenance.

Maybe there should be a "core" group of modules that are downloaded on init and then the rest could be on demand?

@cfriedt cfriedt mentioned this pull request Jan 31, 2023
1 task
@github-actions
Copy link

github-actions bot commented Apr 2, 2023

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants