Skip to content

Conversation

@JianyuWang0623
Copy link
Contributor

@JianyuWang0623 JianyuWang0623 commented Sep 19, 2024

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.

@JianyuWang0623 JianyuWang0623 marked this pull request as draft September 19, 2024 09:50
…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>
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 force-pushed the br_wjy_fix_sensor_ops_s_member branch from 4cc1726 to c71df76 Compare September 19, 2024 09:57
@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
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