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 stm32cube framework #45

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from
Draft

Add stm32cube framework #45

wants to merge 25 commits into from

Conversation

bjsowa
Copy link
Contributor

@bjsowa bjsowa commented Jul 25, 2022

I tested it only on F4. Also I haven't tested the rollover feature, because I don't know how to do it without running the code for 50 days.

@pablogs9
Copy link
Member

This looks good to me.

Could you provide some transport layer such as basic serial transport using STM HAL?

Also, could you integrate this into the CI?

platform: [teensy41, teensy40, teensy36, teensy35, teensy31, due, zero, olimex_e407, esp32dev, nanorp2040connect, portenta_h7_m7, teensy41_eth, nanorp2040connect_wifi, portenta_h7_m7_wifi, esp32dev_wifi, portenta_h7_m7_galactic, portenta_h7_m7_foxy, portenta_h7_m7_rolling, teensy41_custom, pico]

@pablogs9
Copy link
Member

@mergify backport humble foxy galactic

@mergify
Copy link

mergify bot commented Jul 26, 2022

backport humble foxy galactic

🟠 Waiting for conditions to match

  • merged [📌 backport requirement]

@bjsowa
Copy link
Contributor Author

bjsowa commented Aug 1, 2022

Also, could you integrate this into the CI?

I would integrate it with CI but I would need some help. For starters, I have a few questions:

  1. Should I split the ci project into 2 projects (for example: ci/arduino and ci/stm32cube) to separate the code for different frameworks?
  2. Which boards should I choose for the CI?
  3. Should the code be actually functional when flashed to the board or is it enough if it compiles? The stm32cube HAL requires a lot of boilerplate code for setting clocks, peripherals, etc. In my project, I use Stm32CubeMX (through stm32pio) to genarate this code, but I'm not sure how it would work here.

@pablogs9
Copy link
Member

pablogs9 commented Aug 1, 2022

Should I split the ci project into 2 projects (for example: ci/arduino and ci/stm32cube) to separate the code for different frameworks?

Maybe you can add a target here. But if you need to create another folder, it is ok for us.

Which boards should I choose for the CI?

Maybe for initial support, we can add a board that you as a contributor can confirm that works. Just add the needed references in the README.md specifying that this has been tested in X hardware.

Should the code be actually functional when flashed to the board or is it enough if it compiles? The stm32cube HAL requires a lot of boilerplate code for setting clocks, peripherals, etc. In my project, I use Stm32CubeMX (through stm32pio) to generate this code, but I'm not sure how it would work here.

In general, if support for one platform/hardware is added, another user should be able to use it. If your contribution requires further steps to make a basic micro-ROS application works, feel free to add a section in the README.md explaining the dependencies and required steps for making in work.

@bjsowa bjsowa marked this pull request as draft August 1, 2022 12:22
@bjsowa
Copy link
Contributor Author

bjsowa commented Aug 4, 2022

I added an example project and CI job. I didn't want to include generated code so I added a step in CI job that installs CubeMX and stm32pio which I use to generate the code before running platformio.
The example project works on my custom board. Adding another boards should be fairly easy as it should be only a matter of creating a properly configured .ioc project for the board. I don't have any other boards at hand unfortunately.
@pablogs9 any comments?

@pablogs9
Copy link
Member

pablogs9 commented Aug 5, 2022

@Acuadros95 please review

@Acuadros95 Acuadros95 marked this pull request as ready for review August 19, 2022 07:11
Copy link
Contributor

@Acuadros95 Acuadros95 left a comment

Choose a reason for hiding this comment

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

LGTM, will test it on monday.

Also, add the board to the Supported boards list please.

rhead = rtail = thead = thead_next = ttail = 0;
HAL_UART_Receive_DMA(stream->uart, stream->rbuffer, stream->rbuffer_size);
}
open_count++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use a integer for this?

Also, did you encounter any situation where open/close calls miss match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my case, I used rmw_uros_ping_agent periodically to detect agent disconnects. The issue was that this function called open and close on the transport each time it was called, even when the transport was already opened. This is why I decided to ignore all nested open/close calls, hence I use integer to track the nesting level.

It's possible I used the library wrong. Let me know if that's the case or if you know a better way to deal with this problem.

Comment on lines 58 to 62
void HAL_UART_TxCpltCallback(UART_HandleTypeDef* huart) {
if (huart == &huart1) {
uart_transfer_complete_callback(&stream);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this inside of the library?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementing HAL_UART_TxCpltCallback inside the library would be problematic in case the user wants to use another UART port for something else. I don't see a clean way of doing this, any ideas?

.github/workflows/ci.yml Show resolved Hide resolved
Co-authored-by: Antonio Cuadros <[email protected]>
@bjsowa bjsowa marked this pull request as draft September 22, 2022 11:26
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