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

Add Mbed OS support into CHIP #6355

Closed
wants to merge 50 commits into from

Conversation

pan-
Copy link
Contributor

@pan- pan- commented Apr 28, 2021

Problem

There is no support for Mbed OS in CHIP.

Summary of Changes

Add Mbed OS support to CHIP.

It requires the PR #6352 to be merged and the Mbed OS docker image to be pushed on dockerhub to pass the build workflow.

Few details about the port:

  • Our main development target is the PSOC6 WiFi-BT Prototyping kit from cypress:
  • The port only support WiFi and BLE connectivity; not Thread.
  • There is only two examples ported at the moment: Shell and the lock application. More will come as we continue our effort on CHIP.
  • There is good integration with VSCode, build, flash and debug of the examples supported is provided.

There is few commits in this PR which could be their own PRs. I want guidance on what I should do with them:

  • 503f4a1: Prevent mutex initialization if locking is not supported.
  • a4d4181: Avoid use of -Wno-unknown-warning-option in GCC.
  • ec3566b: Update the Android workflow to archive the APK created by the build.
  • b693df4: Small fix to include C99 format macro constant in some files using them.
  • 726ded3: Compilation fix for LwIP 2.

In term of future work here is what the Mbed OS team supporting CHIP will work on next:

  • Unit test support: We're adding Mbed OS support in the test_driver
  • Documentation: This submission is scarce in term of documentation for Mbed OS users we would like to provide a guide that explain how to build a CHIP application with Mbed OS.
  • Example support: We want to support more examples starting with the lightning app then more will follow.
  • Integration testing: It isn't great that no on-device test is run. We've read CHIP on-device testing document but it seems there isn't a lot of movement around it at the moment. We would like to contribute in that area.

Copy link
Contributor

@Damian-Nordic Damian-Nordic left a comment

Choose a reason for hiding this comment

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

Reviewed build files so far.

.github/workflows/examples-mbed.yaml Outdated Show resolved Hide resolved
config/mbed/CMakeLists.txt Show resolved Hide resolved
config/mbed/CMakeLists.txt Outdated Show resolved Hide resolved
config/mbed/CMakeLists.txt Outdated Show resolved Hide resolved
config/mbed/chip-gn/args.gni Outdated Show resolved Hide resolved
examples/lock-app/mbed/main/AppTask.cpp Show resolved Hide resolved
examples/lock-app/mbed/main/AppTask.cpp Outdated Show resolved Hide resolved
examples/lock-app/mbed/main/include/CHIPProjectConfig.h Outdated Show resolved Hide resolved
examples/shell/mbed/CHIPProjectConfig.h Outdated Show resolved Hide resolved
examples/shell/mbed/cmd_mbed_utils.cpp Show resolved Hide resolved
scripts/setup/raspberry_pi/setup_network_env.sh Outdated Show resolved Hide resolved
src/inet/BUILD.gn Outdated Show resolved Hide resolved
src/platform/mbed/BLEManagerImpl.cpp Outdated Show resolved Hide resolved
scripts/requirements.txt Outdated Show resolved Hide resolved
src/inet/BUILD.gn Outdated Show resolved Hide resolved
src/platform/mbed/BLEManagerImpl.cpp Outdated Show resolved Hide resolved
config/mbed/chip-gn/args.gni Outdated Show resolved Hide resolved
)

list(APPEND CHIP_DEFINES
__LINUX_ERRNO_EXTENSIONS__=1
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Is errno task local in Mbed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need that define to access all the error codes required by our BSD socket implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is errno task local in Mbed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure to understand what you mean by task. The errno variable (not really a variable by default with newlib) should be global to the application and there is mechanism in place in newlib and Mbed to handle concurrent access.

@mspang
Copy link
Contributor

mspang commented May 3, 2021

It seems this adds support for Mbed by adding adapters for various POSIX (sockets) and even Linux-specific APIs (eventfds).

Is the aim to just keep using an adapter to POSIX & Linux APIs going forward?

@pan-
Copy link
Contributor Author

pan- commented May 4, 2021

@Damian-Nordic @mspang I think I addressed all your comments. Fixes are in new commit that I can rebase once everything has been reviewed.

Is the aim to just keep using an adapter to POSIX & Linux APIs going forward?

As long as CHIP doesn't expose a platform abstraction for networking, we would prefer keep it that way. We could have chosen to go the LWIP way but this would cut part of our user base out of CHIP if they use an off-board stack and it could cause other issues with part of the user application that wants to use Mbed OS API for networking instead of CHIP API.

The plan is to upstream the BSD socket layer we created to Mbed OS in a near future so this won't stay forever in that repository.

src/platform/mbed/BUILD.gn Outdated Show resolved Hide resolved
@pan- pan- force-pushed the mbed-os-integration branch 3 times, most recently from 422cef1 to be9bd81 Compare May 5, 2021 10:45
@pan-
Copy link
Contributor Author

pan- commented May 5, 2021

@mspang Is there anything else I can do to move this forward ?

src/platform/mbed/ConfigurationManagerImpl.h Show resolved Hide resolved
src/system/SystemConfig.h Outdated Show resolved Hide resolved
@pan- pan- force-pushed the mbed-os-integration branch from fe4198f to d289480 Compare May 6, 2021 13:30
@pan- pan- force-pushed the mbed-os-integration branch from d289480 to 1cced85 Compare May 17, 2021 17:35
@pan-
Copy link
Contributor Author

pan- commented May 17, 2021

I believe the PR address all the comments made.
However the lock app continue to fails though since the last rebase.
#6842 helped but now I see the error reported in: #6867

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 8d7486f

File Section File VM
chip-lighting.elf rodata 104 104
chip-lighting.elf text 104 104
chip-lighting.elf device_handles -8 -8
chip-shell.elf text 24 24
chip-shell.elf device_handles 8 8
chip-shell.elf rodata 8 8
chip-lock.elf rodata 104 104
chip-lock.elf text 104 104
chip-lock.elf device_handles -8 -8
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,1424
.debug_loc,0,571
.debug_str,0,185
.strtab,0,153
rodata,104,104
text,104,104
.symtab,0,96
.debug_ranges,0,48
.debug_abbrev,0,33
.debug_aranges,0,8
.debug_frame,0,4
.shstrtab,0,3
.debug_line,0,-1
device_handles,-8,-8

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,453
.debug_line,0,51
.debug_str,0,32
.debug_loc,0,24
.debug_ranges,0,24
text,24,24
device_handles,8,8
rodata,8,8

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,1410
.debug_loc,0,571
.debug_str,0,185
.strtab,0,153
rodata,104,104
text,104,104
.symtab,0,96
.debug_ranges,0,48
.debug_abbrev,0,33
.debug_aranges,0,8
.debug_frame,0,4
.shstrtab,0,-1
.debug_line,0,-3
device_handles,-8,-8


Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

@mspang Is there anything else I can do to move this forward ?

What's the ETA for landing the POSIX adaptation in your SDK?

"cStandard": "c11",
"cppStandard": "gnu++14",
"intelliSenseMode": "gcc-arm",
"compilerPath": "/opt/mbed-os-toolchain/gcc-arm-none-eabi-9-2019-q4-major/bin/arm-none-eabi-gcc",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this path right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is:

RUN set -x \
&& (mkdir -p /opt/mbed-os-toolchain \
&& cd /opt/mbed-os-toolchain \
&& wget --progress=dot:giga https://developer.arm.com/-/media/Files/downloads/gnu-rm/9-2019q4/RC2.1/gcc-arm-none-eabi-9-2019-q4-major-x86_64-linux.tar.bz2 \
&& tar -xjf gcc-arm-none-eabi-9-2019-q4-major-x86_64-linux.tar.bz2 \
&& rm gcc-arm-none-eabi-9-2019-q4-major-x86_64-linux.tar.bz2 \
&& : ) # last line
# ------------------------------------------------------------------------------
# Configure mbed build system
RUN set -x \
&& mbed config -G GCC_ARM_PATH /opt/mbed-os-toolchain/gcc-arm-none-eabi-9-2019-q4-major/bin/ \
&& mbed toolchain -G -s GCC_ARM \
&& : # last line

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you dedupe this please, we already had a copy of ARM gcc:

COPY --from=nrf /opt/ARM-software/gcc-arm-none-eabi-9-2019-q4-major /opt/ARM-software/gcc-arm-none-eabi-9-2019-q4-major

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will open a PR to modify the docker image.

@pan-
Copy link
Contributor Author

pan- commented May 18, 2021

@mspang I don't have a precise picture on when the BSD socket adaptation will land in Mbed OS. My aim is before the end of the summer.

Is it a concern to have it in the Mbed OS port for now ? We can move it out of CHIP in another repo under the ARMmbed organization as it is independent from CHIP.

@mspang mspang requested a review from kpschoedel May 18, 2021 21:20
@mspang
Copy link
Contributor

mspang commented May 18, 2021

Landing this now risks Mbed becoming the last platform using the select() API for I/O polling. Adapting to a partial implementation of the POSIX APIs we were using puts us in an awkward situation; Mbed may be missing something that we need.

@pan-
Copy link
Contributor Author

pan- commented May 18, 2021

I'm not sure to understand how the problematic is different from Linux or Zephyr which uses select today. Is there a proposed API for #5556 ?
We can collaborate with the individuals handling this issue to make the necessary changes but that would be good to have some visibility on what the change is and when it would land.

@kpschoedel
Copy link
Contributor

Landing this now risks Mbed becoming the last platform using the select() API for I/O polling. Adapting to a partial implementation of the POSIX APIs we were using puts us in an awkward situation; Mbed may be missing something that we need.

I've only been working with the _POSIX implementation, with the intent to convert the _Zephyr version afterward. This version looks substantially similar, so I don't think a third copy makes much difference in that respect. Practically speaking, the immediate platform work is just shuffling some fragments of SysUpdate() and SysProcess().

The first draft API for #5556 was #6561 (see *EndPoint.cpp for examples of use). My current WIP is substantially similar but also encapsulates the file descriptor and active conditions (mPendingIO) to reduce duplication. I hope to put up a revised draft with better API comments tomorrow.

One thing I notice in this PR is a change to the functionality of UDPEndPoint::SendMsg() that doesn't seem mbed-related.

@pan-
Copy link
Contributor Author

pan- commented May 18, 2021

Thanks, I will review #6561.

One thing I notice in this PR is a change to the functionality of UDPEndPoint::SendMsg() that doesn't seem mbed-related.

It addresses and issue in UDPEndPoint::SendMsg. SendMsg can be used directly and it will open a new socket if one hasn't been open:

res = GetSocket(destAddr.Type());
SuccessOrExit(res);
if (mState == kState_Ready)
{
mState = kState_Listening;
// Wake the thread calling select so that it starts selecting on the new socket.
lSystemLayer.WakeSelect();
}

If select is already running, this new socket is not part of the file descriptor being monitored. The call to WakeSelect will force the system to include that socket in the next select call.

@mspang
Copy link
Contributor

mspang commented May 18, 2021

Thanks, I will review #6561.

One thing I notice in this PR is a change to the functionality of UDPEndPoint::SendMsg() that doesn't seem mbed-related.

It addresses and issue in UDPEndPoint::SendMsg. SendMsg can be used directly and it will open a new socket if one hasn't been open:

res = GetSocket(destAddr.Type());
SuccessOrExit(res);
if (mState == kState_Ready)
{
mState = kState_Listening;
// Wake the thread calling select so that it starts selecting on the new socket.
lSystemLayer.WakeSelect();
}

If select is already running, this new socket is not part of the file descriptor being monitored. The call to WakeSelect will force the system to include that socket in the next select call.

These generic bug fixes or other uncoupled changes should really be separated out into standalone PRs.

@stale
Copy link

stale bot commented May 27, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label May 27, 2021
@stale
Copy link

stale bot commented Jun 3, 2021

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Jun 3, 2021
@mspang mspang reopened this Jun 24, 2021
@stale stale bot removed the stale Stale issue or PR label Jun 24, 2021
@mspang
Copy link
Contributor

mspang commented Jun 24, 2021

@pan- Your PR was approved, are you able to rebase it?

@woody-apple
Copy link
Contributor

/rebase

@pan-
Copy link
Contributor Author

pan- commented Jun 24, 2021

@mspang I was planning in making a new one since that was was marked at stalled while the BLE flow was broken...
It is fixed here now and we kept rebasing our port. I can push on this PR the update for sure. However few things have changed since the review:

  • Our posix adaptation layer has been moved in another repository
  • MDNS has been enabled for UDP.

We also support more examples but I will open new PRs for them.

@mspang
Copy link
Contributor

mspang commented Jun 25, 2021

@mspang I was planning in making a new one since that was was marked at stalled while the BLE flow was broken...
It is fixed here now and we kept rebasing our port. I can push on this PR the update for sure. However few things have changed since the review:

  • Our posix adaptation layer has been moved in another repository
  • MDNS has been enabled for UDP.

We also support more examples but I will open new PRs for them.

Ok, opening a new PR sounds fine.

@mspang mspang closed this Jun 25, 2021
@pan- pan- mentioned this pull request Jun 25, 2021
@ATmobica ATmobica deleted the mbed-os-integration branch May 11, 2022 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants