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 Spresense board support #323

Open
wants to merge 2 commits into
base: foxy
Choose a base branch
from

Conversation

Barry-Xu-2018
Copy link

No description provided.

Signed-off-by: Barry Xu <[email protected]>

Co-authored-by: Chen Lihui <[email protected]>
@pablogs9 pablogs9 requested review from Acuadros95 and removed request for jamoralp and FranFin May 25, 2021 06:56
@Acuadros95
Copy link
Contributor

Hello, just two comments:

  • We didn't expect modifications on the micro_ros_setup general scripts (build_firmware.sh, configure_firmware.sh ...) did you encounter problems with them?
  • It could be more interesting to integrate your board on the new micro-ROS Nuttx app, which is not related to micro_ros_setup.

@Barry-Xu-2018
Copy link
Author

@Acuadros95

Thanks for your comments.

  • We didn't expect modifications on the micro_ros_setup general scripts (build_firmware.sh, configure_firmware.sh ...) did you encounter problems with them?

Yes. I understand. These scrips (build_firmware.sh, etc) should not be modified.
But current logic of codes is a little confused to me. So I modify it and want to discuss it with reviewer.
e.g.

if [ $PLATFORM != "generic" ] && [ -d "$PREFIX/config/$RTOS/generic" ]; then
. $PREFIX/config/$RTOS/generic/build.sh
else
. $PREFIX/config/$RTOS/$PLATFORM/build.sh
fi

generic directory put common scripts which may be used for different platform.
$PLATFORM directory put special scripts which is only for this platform.
Naturally,if build.sh exist in $PLATFORM directory, this script should be used instead of flash.sh in generic directory.
Current logic is if flash.sh exists in generic directory, it should be used. If I want to special operations in flash.sh for my platform, how to do ? (generic directory should be removed ?)

  • It could be more interesting to integrate your board on the new micro-ROS Nuttx app, which is not related to micro_ros_setup.

I know you plan to remove official support to NuttX in micro-ROS Galactic. So foxy version also don't want to add any change for NuttX. Right ?

@Acuadros95
Copy link
Contributor

Yes, you are right, $PLATFORM directory should be preferred over generic. I will check that these changes don't affect the rest of the available platforms.

And we will keep supporting NuttX on foxy, so feel free to include your port into this version. But in the future, only the new app will be used.

@Barry-Xu-2018
Copy link
Author

Yes, you are right, $PLATFORM directory should be preferred over generic. I will check that these changes don't affect the rest of the available platforms.

Okay. Thank you.

And we will keep supporting NuttX on foxy, so feel free to include your port into this version. But in the future, only the new app will be used.

Get it.

README.md Outdated
@@ -138,6 +141,7 @@ In summary, the supported configurations for transports are:
| ST Nucleo H743ZI <sup>1</sup> | - | - | UART |
| ST Nucleo F746ZG <sup>1</sup> | - | UART | UART |
| ST Nucleo F767ZI <sup>1</sup> | - | UART | - |
| Spresense | WiFi | - | - |

Choose a reason for hiding this comment

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

Nit:

Suggested change
| Spresense | WiFi | - | - |
| Spresense | Network | - | - |

set -o pipefail


# spresense use own sdk to config

Choose a reason for hiding this comment

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

Nit:

Suggested change
# spresense use own sdk to config
# spresense uses own sdk to config

<name>firmware</name>
<version>0.0.1</version>
<description>This is a dummy package to supply firmware build dependencies for spresense.</description>
<maintainer email="[email protected]">Ingo Luetkebohle</maintainer>

Choose a reason for hiding this comment

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

Suggested change
<maintainer email="[email protected].com">Ingo Luetkebohle</maintainer>
<maintainer email="Barry.Xu@sony.com">Barry Xu</maintainer>
<maintainer email="[email protected]">Tomoya Fujita</maintainer>

Copy link
Author

Choose a reason for hiding this comment

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

@fujitatomoya

Thanks for your review comments. I have addressed them at 4184ad9.

Signed-off-by: Barry Xu <[email protected]>
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