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.

Sign up for GitHub

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

Jump to bottom

subsys: Add MCTP as a subsystem, built on libmctp with bindings #75743

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

subsys: Add MCTP as a subsystem, built on libmctp with bindings #75743

wants to merge 4 commits into from
+711 −0

Conversation

teburd
Copy link
Collaborator

@teburd teburd commented Jul 11, 2024 •
edited
Loading

Adds libmctp as a zephyr module from my fork with a few small changes I intend on trying to upstream along with a binding to Zephyr's polling async UART API. Comes with a pair of sample applications that can be used on two boards connected via UART.

@teburd teburd changed the title Libmctp module modules: libmctp Jul 11, 2024
@teburd teburd requested review from nashif and KeHIntel July 11, 2024 00:48
@zephyrbot
Copy link
Collaborator

zephyrbot commented Jul 11, 2024 •
edited
Loading

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
libmctp 🆕 N/A (Added) zephyrproject-rtos/libmctp@fa15266 (zephyr) N/A

⛔ DNM label due to: 1 added project

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

@zephyrbot zephyrbot added manifest manifest-libmctp DNM This PR should not be merged (Do Not Merge) labels Jul 11, 2024
@teburd teburd added this to the v4.0.0 milestone Jul 11, 2024
@ubieda ubieda self-requested a review July 31, 2024 16:24
@teburd teburd force-pushed the libmctp_module branch 6 times, most recently from e32c677 to 44752b4 Compare August 8, 2024 15:47
@teburd teburd force-pushed the libmctp_module branch 2 times, most recently from bd7c893 to 153b02d Compare August 14, 2024 20:17
@teburd
Copy link
Collaborator Author

teburd commented Aug 14, 2024

Need to work out some upstream stuff, but can then have the fork setup in the zephyrproject-rtos github org

@teburd teburd added the TSC Topics that need TSC discussion label Aug 21, 2024
@teburd teburd mentioned this pull request Aug 21, 2024
Open
@teburd teburd force-pushed the libmctp_module branch 4 times, most recently from 91652d2 to f884e0d Compare August 22, 2024 15:46
@teburd
Copy link
Collaborator Author

teburd commented Aug 22, 2024

In my original samples I had hacked up the serial binding to work with Zephyr natively, to better integrate with Zephyr however its useful to provide native bindings in a subsys so that's what I've begun to do.

henrikbrixandersen
henrikbrixandersen reviewed Sep 4, 2024
View reviewed changes
doc/Makefile Outdated Show resolved Hide resolved
samples/modules/mctp/mctp_endpoint/CMakeLists.txt Outdated Show resolved Hide resolved
@teburd teburd force-pushed the libmctp_module branch 2 times, most recently from 29717fb to e1f9cbd Compare September 6, 2024 16:36
dkalowsk
dkalowsk previously approved these changes Dec 27, 2024
View reviewed changes
Copy link
Contributor

@dkalowsk dkalowsk left a comment

Choose a reason for hiding this comment

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

I've got a few minor suggestions, but nothing that should stop this from moving forward. Approving on that end.

subsys/mctp/Kconfig Show resolved Hide resolved
subsys/mctp/mctp_uart.c Outdated Show resolved Hide resolved
subsys/mctp/mctp_uart.c Outdated Show resolved Hide resolved
subsys/mctp/mctp_uart.c Show resolved Hide resolved
kartben
kartben reviewed Dec 27, 2024
View reviewed changes
samples/modules/mctp/mctp_endpoint/README.md Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do .rst :) also please follow the code sample template https://github.com/zephyrproject-rtos/zephyr/blob/main/doc/templates/sample.tmpl so this properly shows in the documentation. You'll also need a page similar to https://github.com/zephyrproject-rtos/zephyr/blob/main/samples/modules/lvgl/lvgl.rst in the mctp folder to define a new sample category for MCTP samples

Copy link
Collaborator Author

@teburd teburd Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The sample here is really the pair of firmwares being built and run on two boards. Do we have anything like that in the tree already that you know of?

Really this is one sample with two different firmwares. Really unsure of how best to structure it but documenting them separately actually doesn't make much sense.

Converting to rst is of course no problem, but do note we have markdown readme files in the tree for samples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think its good now, let me know

@zephyrbot zephyrbot added the area: llext Linkable Loadable Extensions label Dec 27, 2024
@zephyrbot zephyrbot requested review from lyakh and pillo79 December 27, 2024 16:25
@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Dec 27, 2024
@teburd teburd force-pushed the libmctp_module branch 4 times, most recently from b414f5c to 437db75 Compare December 27, 2024 18:36
@teburd teburd requested review from dkalowsk and kartben December 27, 2024 18:36
@dkalowsk
Copy link
Contributor

seems to have picked up an extra commit for renaming llext_simple_test and not sure if that is correct. The MCTP changes all look good to me. Approval held off due to confusion on the extra commit.

@teburd
modules: Add libmctp as a module
817dbd2 
Adds libmctp as a west module dependency

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
@teburd
mctp: Add mctp subsystem with uart binding
aaa6a44 
libmctp provides interfaces for wiring up a MCTP bus it calls bus
bindings. The bindings provided in libmctp however are not directly
useful to Zephyr without some work. Provide an initial uart binding that
directly uses Zephyr's async uart interface.

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
@dkalowsk dkalowsk removed the area: llext Linkable Loadable Extensions label Dec 27, 2024
@teburd teburd removed the DNM This PR should not be merged (Do Not Merge) label Dec 27, 2024
@teburd
Copy link
Collaborator Author

teburd commented Dec 27, 2024

seems to have picked up an extra commit for renaming llext_simple_test and not sure if that is correct. The MCTP changes all look good to me. Approval held off due to confusion on the extra commit.

Oops! yeah had committed that to my local main, and now all my rebases have it, argh, gotta go review other PRs as well.

dkalowsk
dkalowsk previously approved these changes Dec 27, 2024
View reviewed changes
@dkalowsk dkalowsk linked an issue Dec 27, 2024 that may be closed by this pull request
Module for libmctp #77351
Open
@teburd
samples: Add mctp host and client samples
c60d37f 
Samples work by sending MCTP encoded messages over a uart between two
boards.

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
@teburd
MAINTAINERS.yml: Add libmctp module maintainership
7db91ef 
Adds myself, nashif, and inteljiangwe1 to the maintainers file covering
the libmctp module.

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Dec 30, 2024
@teburd teburd removed the DNM This PR should not be merged (Do Not Merge) label Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henrikbrixandersen henrikbrixandersen henrikbrixandersen left review comments

@dkalowsk dkalowsk dkalowsk approved these changes

@nashif nashif Awaiting requested review from nashif

@ubieda ubieda Awaiting requested review from ubieda

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@stephanosio stephanosio Awaiting requested review from stephanosio

@KeHIntel KeHIntel Awaiting requested review from KeHIntel

@lyakh lyakh Awaiting requested review from lyakh

@pillo79 pillo79 Awaiting requested review from pillo79

@kartben kartben Awaiting requested review from kartben

Requested changes must be addressed to merge this pull request.

Assignees

@kartben kartben

Labels
area: Process area: Samples Samples manifest manifest-libmctp
Projects
TSC Attention Needed
Status: Done
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.

Module for libmctp
7 participants
@teburd @zephyrbot @KeHIntel @dkalowsk @kartben @henrikbrixandersen @mmahadevan108