linguini1 commented on PR #18266: URL: https://github.com/apache/nuttx/pull/18266#issuecomment-3841295818
> 1、For AI-generated PR The community requires too much information to describe in detail for every patch. Respectfully, those are the requirements. We voted on them as a community and they are in the contributing guidelines. I don't know what to tell you if you (as the developer of your change) can't write a brief summary of what it does, what subsystem it impacts and include a log from your testing showing that it does what you say it does without copy-pasting your patch into an LLM. If you have a problem with the PR requirements, please start a discussion on the dev mailing list and perhaps the community will consider re-voting on this issue. Until then, at least verify the output of your AI-generated descriptions. It's disrespectful to reviewers to generate AI descriptions and paste them while we need to actually assess your code and get it to a state where it can be merged. > 2、deadlock The call stack has already been posted in the deadlock submission, so I won't elaborate further. If you believe the call stack was generated by AI, then I have nothing more to say. Can you please at least link to them from your description in the future? There's no mention of where to find those logs in your PR, how am I supposed to guess where the logs are? > drivers/sensors: fix deadlock about upper lock between user task and rptun thread. > > ``` > call trace: > user task: > > nxsig_usleep > nuttx/arch/arm/src/../../../sched/signal/sig_usleep.c:82 > rpmsg_virtio_get_tx_payload_buffer > nuttx/arch/arm/src/../../../openamp/open-amp/lib/rpmsg/rpmsg_virtio.c:408 > rpmsg_get_tx_payload_buffer.constprop.0 > nuttx/arch/arm/src/../../../openamp/open-amp/lib/rpmsg/rpmsg.c:223 > sensor_rpmsg_advsub_one > nuttx/arch/arm/src/../../../drivers/sensors/sensor_rpmsg.c:306 > sensor_rpmsg_advsub > nuttx/arch/arm/src/../../../drivers/sensors/sensor_rpmsg.c:338 > sensor_rpmsg_open > nuttx/arch/arm/src/../../../drivers/sensors/sensor_rpmsg.c:641 > sensor_open > nuttx/arch/arm/src/../../../drivers/sensors/sensor.c:576 > file_vopen > nuttx/arch/arm/src/../../../fs/vfs/fs_open.c:248 > nx_vopen > nuttx/arch/arm/src/../../../fs/vfs/fs_open.c:307 > open > nuttx/arch/arm/src/../../../fs/vfs/fs_open.c:465 > orb_advsub_open > nuttx/arch/arm/src/../../../../apps/system/uorb/uORB/uORB.c:69 > orb_subscribe_multi > > rptun task: > > nuttx/include/arch/syscall.h:179 > nxsem_wait > nuttx/arch/arm/src/../../../sched/semaphore/sem_wait.c:176 > nxmutex_lock > nuttx/arch/arm/src/../../../libs/libc/misc/lib_mutex.c:233 > nxrmutex_lock > nuttx/arch/arm/src/../../../libs/libc/misc/lib_mutex.c:580 > sensor_lock > nuttx/arch/arm/src/../../../drivers/sensors/sensor.c:207 > sensor_rpmsg_lock > nuttx/arch/arm/src/../../../drivers/sensors/sensor_rpmsg.c:289 > sensor_rpmsg_unsub_handler > nuttx/arch/arm/src/../../../drivers/sensors/sensor_rpmsg.c:1087 > sensor_rpmsg_ept_cb > nuttx/arch/arm/src/../../../drivers/sensors/sensor_rpmsg.c:1229 > rpmsg_virtio_rx_callback > nuttx/arch/arm/src/../../../openamp/open-amp/lib/rpmsg/rpmsg_virtio.c:624 > ``` Honestly without explanation I have no idea what this is proving with regards to deadlock, it's just a callstack. Can you link to the other patch so I may have a read? > 3、power save Regarding power saving: 20-30 is a rough estimate, depending on your scenario. My scenario achieves this figure, or even better. > > * For example, UORB is used for communication between Vela and Android. In standby scenarios, after Android goes to sleep, the subscribed light brightness will not be sent to the Android big core. At this time, the power consumption difference between Android and Vela small cores is self-evident. For example, the difference between Qualcomm 8Gen4 and ESP 32S3 is quite obvious. Okay, this is a better explanation! It still has no numbers from your testing but I think this would have been a better choice for in the PR description. Reviewers appreciate knowing the test setup. > * UORB broadcasting scenarios. One characteristic of UORB is that it initiates a broadcast when there are subscribers or advertisers. In this case, the remote core, which is asleep, will be woken up by the local core. Wakeup optimizes this scenario; it wakes up the node on the first broadcast, and subsequent cancellations do not disconnect the link or release the node. This significantly reduces the number of wakeups in scenarios with frequent broadcasts, saving considerable power. The exact percentage depends on your specific scenario; I'm giving around 20%. Again, a good explanation. I have no idea how you came up with 20% without actually checking the power consumption before and after, but I'm not going to argue this point further. My change request has since been dismissed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
