Skip to content

Conversation

@zyfeier
Copy link
Contributor

@zyfeier zyfeier commented Dec 4, 2024

Summary

Change netdev_lower_quota_load to macro

Impact

netdev

Testing

sim:citest pass

@github-actions github-actions bot added Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small labels Dec 4, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 4, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details. Here's a breakdown:

  • Insufficient Summary: "Change netdev_lower_quota_load to macro" is too brief. Why was this change made? What problem does it solve or what improvement does it offer? How does changing it to a macro work (what's the macro definition)? Are there any related issues?

  • Insufficient Impact: "netdev" is too vague. Which specific network drivers or subsystems are affected? All the "Impact" questions need to be answered explicitly (NO/YES and description if YES). Even if the answer is NO, stating it explicitly is important for clarity. For example:

    • Impact on user: NO
    • Impact on build: Potentially YES (if the macro changes header file dependencies). Please describe.
    • ...and so on for all the impact categories.
  • Insufficient Testing: "sim:citest pass" is not enough. What specific sim target and configuration were used? What host OS and compiler? Critically, there are no "before" and "after" logs provided to demonstrate the change's effect. While a passing CI test is good, it's not a substitute for showing the logs demonstrating what was tested and how the behavior changed.

In short, the PR needs to be much more descriptive and provide concrete details for each of the required sections. Simply stating keywords is not sufficient. Reviewers need context to understand the change, its motivations, and its effects.

@acassis
Copy link
Contributor

acassis commented Dec 4, 2024

There are some issues on esp32c3:

Configuration/Tool: esp32c3-devkit/ble
2024-12-04 14:17:55
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
In file included from bluetooth/bt_conn.c:51:
bluetooth/bt_conn.c: In function 'bt_conn_addref':
Error: bluetooth/bt_conn.c:765:10: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'bt_atomic_t' {aka 'long int'} [-Werror=format=]
  765 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |          ^~~~~~~~~~~~~~~~~~~~
bluetooth/bt_conn.c:765:26: note: format string is defined here
  765 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |                         ~^
      |                          |
      |                          unsigned int
      |                         %lu
bluetooth/bt_conn.c: In function 'bt_conn_release':
Error: bluetooth/bt_conn.c:790:10: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'bt_atomic_t' {aka 'long int'} [-Werror=format=]
  790 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |          ^~~~~~~~~~~~~~~~~~~~
bluetooth/bt_conn.c:790:26: note: format string is defined here
  790 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |                         ~^
      |                          |
      |                          unsigned int
      |                         %lu
cc1: all warnings being treated as errors
make[1]: *** [Makefile:46: bt_conn.o] Error 1
make[1]: Target 'libwireless.a' not remade because of errors.

@fdcavalcanti
Copy link
Contributor

There are some issues on esp32c3:

Configuration/Tool: esp32c3-devkit/ble
2024-12-04 14:17:55
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
In file included from bluetooth/bt_conn.c:51:
bluetooth/bt_conn.c: In function 'bt_conn_addref':
Error: bluetooth/bt_conn.c:765:10: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'bt_atomic_t' {aka 'long int'} [-Werror=format=]
  765 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |          ^~~~~~~~~~~~~~~~~~~~
bluetooth/bt_conn.c:765:26: note: format string is defined here
  765 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |                         ~^
      |                          |
      |                          unsigned int
      |                         %lu
bluetooth/bt_conn.c: In function 'bt_conn_release':
Error: bluetooth/bt_conn.c:790:10: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'bt_atomic_t' {aka 'long int'} [-Werror=format=]
  790 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |          ^~~~~~~~~~~~~~~~~~~~
bluetooth/bt_conn.c:790:26: note: format string is defined here
  790 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |                         ~^
      |                          |
      |                          unsigned int
      |                         %lu
cc1: all warnings being treated as errors
make[1]: *** [Makefile:46: bt_conn.o] Error 1
make[1]: Target 'libwireless.a' not remade because of errors.

Just got this on CI as well.

Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
@zyfeier
Copy link
Contributor Author

zyfeier commented Dec 5, 2024

image
This seems like a Git permission error, It wasn't caused by this modification. @xiaoxiang781216

@xiaoxiang781216
Copy link
Contributor

let's ignore the ci download error, and merge this patch to fix the known compiler error.

@xiaoxiang781216 xiaoxiang781216 merged commit 060fda0 into apache:master Dec 5, 2024
24 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Bluetooth Area: Networking Effects networking subsystem Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants