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 RPI Pico SDK mutex and locking to multithread #345

Open
wants to merge 8 commits into
base: ros2
Choose a base branch
from

Conversation

DwayneGit
Copy link

Implemented locking for rpi pico sdk per our discussion here:
micro-ROS/micro_ros_raspberrypi_pico_sdk#795

Sorry for PR spam. This is my first time using sign off

pablogs9 and others added 8 commits November 30, 2021 15:39
* Add Twitter and Readthedocs shields

Signed-off-by: Pablo Garrido <[email protected]>

* Update

Signed-off-by: Pablo Garrido <[email protected]>

* Fix

Signed-off-by: Pablo Garrido <[email protected]>
(cherry picked from commit 8b78d22)

Co-authored-by: Pablo Garrido <[email protected]>
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@@ -32,6 +32,8 @@ struct uxrSession;
#elif defined(PLATFORM_NAME_FREERTOS)
#include "FreeRTOS.h"
#include "semphr.h"
#elif defined(PLATFORM_NAME_RPIPICO)
Copy link
Member

Choose a reason for hiding this comment

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

Where is PLATFORM_NAME_RPIPICO defined?

Copy link
Author

Choose a reason for hiding this comment

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

I gathered from here #233 that PLATFORM_NAME_FREERTOS was defined by the user so followed the same logic. I stuck add_compile_definitions(PLATFORM_NAME_RPIPICO) at the bottom of the toolchain.cmake file and it works however the its not detecting the "pico/mutex.h" so I am working on linking that.

Copy link
Member

Choose a reason for hiding this comment

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

You will need to add the include folders of Pico SDK on the CMake toolchain.

Copy link
Author

@DwayneGit DwayneGit Feb 23, 2023

Choose a reason for hiding this comment

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

Yeah, I am working on that. The problem is the sdk has all of its include directories scattered and one file is "version.h" is auto-generated and stored in the build folder. I gathered most of what i think is important and trying to compile now but running into some errors that i don't understand like:

/home/.../pico/pico-sdk/src/rp2040/hardware_structs/include/hardware/structs/timer.h:105:33: error: expected declaration specifiers or '...' before string constant
  105 | static_assert( NUM_TIMERS == 4, "");
include_directories(
    $ENV{PICO_SDK_PATH}/build/generated/pico_base
    $ENV{PICO_SDK_PATH}/src/rp2040/hardware_regs/include
    $ENV{PICO_SDK_PATH}/src/rp2040/hardware_structs/include
    $ENV{PICO_SDK_PATH}/src/rp2_common/hardware_timer/include
    $ENV{PICO_SDK_PATH}/src/rp2_common/hardware_base/include
    $ENV{PICO_SDK_PATH}/src/rp2_common/hardware_sync/include
    $ENV{PICO_SDK_PATH}/src/rp2_common/pico_platform/include
    $ENV{PICO_SDK_PATH}/src/common/pico_base/include
    $ENV{PICO_SDK_PATH}/src/common/pico_time/include
    $ENV{PICO_SDK_PATH}/src/common/pico_sync/include
)

If I comment that line out it works but I'm looking for a better solution than that

Copy link
Author

Choose a reason for hiding this comment

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

It looks like the problem is the code is being compiled with -std=c99 somewhere but the pico sdk needs c11 to compile with the static_assert function. I cloned the sdk repository branch and patched that and am able to build just fine.

Copy link
Author

@DwayneGit DwayneGit Feb 23, 2023

Choose a reason for hiding this comment

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

Actually adding "-DUCLIENT_C_STANDARD=11" to the colcon.meta solved this problem for the microxrcedds_client but i get the same error in rmw_microxrcedds where it looks like the C_STANDARD is hard coded to 99. If this CMakeList file could change that static 99 to an argument with a default of 99 like UCLIENT_C_STANDARD here i think this would not be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, we will merge this asap

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

Successfully merging this pull request may close these issues.

3 participants