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

Enhancement for Rust build #3020

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

Enhancement for Rust build #3020

wants to merge 2 commits into from

Conversation

no1wudi
Copy link
Contributor

@no1wudi no1wudi commented Mar 5, 2025

Summary:

  • Update the NuttX Rust crate dependency to use the main branch instead of master for consistency with upstream repository changes.
  • Fix framebuffer pixel format check by replacing Format::RGB565 with FB_FMT_RGB16_565 to ensure accurate validation of the video format.

nuttx-rs (https://github.com/no1wudi/nuttx-rs) generate binding from NuttX's C header now, which simplify the build setup and get more accurate struct/data type definitions.

Impact:

  • New feature added? NO
  • Impact on user? NO
  • Impact on build? YES - Updates dependency branch in Cargo.toml, affecting crate resolution.
  • Impact on hardware? NO
  • Impact on documentation? NO
  • Impact on security? NO
  • Impact on compatibility? NO

This PR ensures compatibility with the latest NuttX Rust crate repository structure and corrects a critical pixel format check for reliable framebuffer handling.

no1wudi added 2 commits March 5, 2025 20:48
Set NUTTX_INCLUDE_DIR in the Rust build commands to include the necessary directories for proper compilation.
* Ensured correct include paths for Rust projects
* Improved build reliability for Rust components

Signed-off-by: Huang Qi <[email protected]>
…d fix pixel format check.

Changed the NUTTX dependency branch to 'main' for consistency with repository updates.
Updated pixel format check to use FB_FMT_RGB16_565 for better clarity and accuracy.

Signed-off-by: Huang Qi <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Mar 5, 2025

[Experimental Bot, please feedback here]

Yes, this PR generally meets the NuttX requirements, although it's missing some key information in the Testing section. While it describes the changes and their impact well, it needs to provide specific build hosts and targets used for verification, along with actual testing logs. The reference to nuttx-rs seems out of place in the Summary and should probably be part of the description of "how the change exactly works".

Here's a breakdown of what's good and what needs improvement:

Good:

  • Clear Summary: Explains the "why" and "what" of the changes.
  • Well-Defined Impact: Addresses all impact categories with clear YES/NO answers and explanations where necessary.
  • Concise Explanation: Provides a good overview of the benefits of the changes.

Needs Improvement:

  • Testing Section: This is the most significant deficiency.
    • Missing Build Hosts: Specify the OS, CPU architecture, and compiler used for local testing. Example: Linux, x86_64, GCC 12.1.0
    • Missing Targets: Specify the target architecture, board, and configuration. Example: sim:qemu-x86_64:nsh
    • Missing Logs: Provide actual logs demonstrating the issue before the change and the corrected behavior after the change. Even simple logs showing successful framebuffer initialization or output would be helpful. Without these logs, it's impossible to verify the claims made in the PR.
  • Summary Clarity: Move the nuttx-rs explanation into the "how it works" part of the summary. The current placement makes it seem like a separate change. Rephrase to something like: "This change updates the NuttX Rust crate dependency... and corrects the framebuffer pixel format check... The nuttx-rs crate now generates bindings..."

By providing the missing testing details, this PR will fully meet the NuttX requirements.

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @no1wudi :-)

@cederom cederom requested a review from lupyuen March 5, 2025 15:34
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.

4 participants