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.

Sign up for GitHub

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

Jump to bottom

nuttx/uorb: Fix build error #13540

Merged
merged 2 commits into from
Sep 19, 2024
Merged

nuttx/uorb: Fix build error #13540

merged 2 commits into from
Sep 19, 2024

Conversation

JianyuWang0623
Copy link
Contributor

@JianyuWang0623 JianyuWang0623 commented Sep 19, 2024 •
edited
Loading

Summary

New member added to struct sensor_ops_s, and type changed, but not update all existed code, for details, please see "Related change" below.

  • Error 1:
  1153  Building NuttX...
  1154Error: sensors/bme680_uorb.c:428:3: error: initialization of 'int (*)(struct sensor_lowerhalf_s *, struct file *, long unsigned int)' from incompatible pointer type 'int (*)(struct sensor_lowerhalf_s *, struct file *, int,  long unsigned int)' [-Werror=incompatible-pointer-types]
  1155  428 |   bme680_control    /* control */
  1156      |   ^~~~~~~~~~~~~~
  • Error 2:
      Error: sensors/ms56xx_uorb.c:145:20: error: initialization of 'int (*)(struct sensor_lowerhalf_s *, struct file *, uint32_t *)' {aka 'int (*)(struct sensor_lowerhalf_s *, struct file *, unsigned int *)'} from incompatible pointer type 'int (*)(struct sensor_lowerhalf_s *, struct file *, long unsigned int *)' [-Werror=incompatible-pointer-types]
        145 |   .set_interval  = ms56xx_set_interval,
            |                    ^~~~~~~~~~~~~~~~~~~
      sensors/ms56xx_uorb.c:145:20: note: (near initialization for 'g_sensor_ops.set_interval')
      cc1: all warnings being treated as errors

Impact

drivers/sensors

Testing

./tools/configure.sh -l esp32-devkitc:ms5611
./tools/configure.sh -l esp32-sparrow-kit:nsh

esp32-devkitc:ms5611 and esp32-sparrow-kit:nsh compiled ok, but failed in link stage(maybe because of abnormal local env), waiting for result of CI(xtensa-01).

@nuttxpr
Copy link

nuttxpr commented Sep 19, 2024

[Experimental Bot, please feedback here]

Squash The Commits: This PR contains 2 Commits. Please Squash the Multiple Commits into a Single Commit.

NuttX PR Requirements Review

This PR appears incomplete and does not meet the NuttX requirements. Here's why:

Summary:

  • While the summary describes the technical changes, it lacks a clear explanation of why these changes are necessary. What problem does this solve?
  • The summary mentions errors encountered but doesn't elaborate on the root cause.
  • It's unclear what "existed code" wasn't updated and why.

Impact:

  • Incomplete: The impact section only mentions "drivers/sensors" but needs a more detailed analysis.
  • User Impact: Will users need to modify their applications due to the type changes?
  • Build Impact: The errors indicate a build impact. The description should explain how the build process is affected and if any specific configurations are impacted.
  • Hardware Impact: Are there any specific architectures, boards, or drivers affected by these changes?
  • Documentation: Is documentation impacted? If so, has it been updated?
  • Other impacts (Security, Compatibility): These are not addressed and should be analyzed.

Testing:

  • Insufficient: The PR acknowledges a lack of testing due to unavailable hardware. While CI compilation is helpful, it doesn't replace actual functional testing.
  • Missing Logs: The "Testing logs before change" and "Testing logs after change" sections are empty.

Recommendations:

  1. Explain the "Why": Clearly articulate the reason and benefits of these changes.
  2. Root Cause Analysis: Explain why the existing code wasn't updated and the challenges involved.
  3. Complete Impact Analysis: Thoroughly analyze and document the impact on all aspects (users, build, hardware, documentation, security, compatibility).
  4. Testing Plan: If hardware is unavailable, outline a testing plan for when it's accessible. Consider alternative testing methods like simulation or mocking.
  5. Provide Context: Give enough information for reviewers to understand the changes without needing to look at the linked commits.

By addressing these points, you'll significantly improve the PR's quality and facilitate a smoother review process.

@lupyuen lupyuen mentioned this pull request Sep 19, 2024
Merged
@JianyuWang0623 JianyuWang0623 marked this pull request as draft September 19, 2024 09:50
@JianyuWang0623
nuttx/uorb: Fix initialization error of variable of type `struct sens…
bde2ac0 
…or_ops_s`

Related change:
  .flush    : 4034693
  .get_info : 703bb7e

Error Log:
  1153  Building NuttX...
  1154Error: sensors/bme680_uorb.c:428:3: error: initialization of 'int (*)(struct sensor_lowerhalf_s *, struct file *, long unsigned int)' from incompatible pointer type 'int (*)(struct sensor_lowerhalf_s *, struct file *, int,  long unsigned int)' [-Werror=incompatible-pointer-types]
  1155  428 |   bme680_control    /* control */
  1156      |   ^~~~~~~~~~~~~~

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
@JianyuWang0623
nuttx/uorb: Fix incompatible type error
c71df76 
Related: b17c074

Log:
  Error: sensors/ms56xx_uorb.c:145:20: error: initialization of 'int (*)(struct sensor_lowerhalf_s *, struct file *, uint32_t *)' {aka 'int (*)(struct sensor_lowerhalf_s *, struct file *, unsigned int *)'} from incompatible pointer type 'int (*)(struct sensor_lowerhalf_s *, struct file *, long unsigned int *)' [-Werror=incompatible-pointer-types]
    145 |   .set_interval  = ms56xx_set_interval,
        |                    ^~~~~~~~~~~~~~~~~~~
  sensors/ms56xx_uorb.c:145:20: note: (near initialization for 'g_sensor_ops.set_interval')
  cc1: all warnings being treated as errors

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
@JianyuWang0623 JianyuWang0623 marked this pull request as ready for review September 19, 2024 10:00
@xiaoxiang781216 xiaoxiang781216 merged commit bdddc76 into apache:master Sep 19, 2024
20 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JianyuWang0623 @nuttxpr @xiaoxiang781216