Skip to content

Conversation

@yamt
Copy link
Contributor

@yamt yamt commented Sep 27, 2024

Summary

This reverts commit 35c8c80.

because it caused a regression.
with esp32-devkitc:knsh, nsh prompt on serial console doesn't seem to accept any input.

Impact

Testing

tested with with esp32-devkitc:knsh

This reverts commit 35c8c80.

because it caused a regression.
with esp32-devkitc:knsh, nsh prompt on serial console doesn't seem to
accept any input.
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: ceva Issues related to CEVA architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Sep 27, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 27, 2024

[Experimental Bot, please feedback here]

The PR summary is extremely minimal. Here's a breakdown and suggestions for improvement:

What's good:

  • It states the PR is a revert.
  • It links the commit being reverted.
  • It mentions the regression observed ("nsh prompt... doesn't accept input").
  • It specifies the platform where testing occurred.

What needs improvement:

  • Missing "Why": While stating it's a regression implies the change was detrimental, be more explicit. Was the original commit flawed? Did it break something unexpectedly? This adds context for reviewers.
  • Vague Regression: "Doesn't accept input" is broad. Does nothing happen? Are characters echoed strangely? More detail helps pinpoint the issue.
  • No "Before" State: Testing logs are empty. To prove a regression, show how the system behaved before the bad commit, then after. This demonstrates the change's impact.

Concisely, the PR does NOT meet NuttX requirements sufficiently. It's on the right track but lacks detail to make it easily reviewable.

@GUIDINGLI
Copy link
Contributor

#13662

@GUIDINGLI
Copy link
Contributor

@yamt
Can you try this PR:
#13662

@yamt
Copy link
Contributor Author

yamt commented Sep 27, 2024

@yamt Can you try this PR: #13662

i don't think i will test it anytime soon.
please test it by yourself or merge this revert. (i tested this)

@GUIDINGLI
Copy link
Contributor

You can close this PR now, thanks

@GUIDINGLI GUIDINGLI self-requested a review September 27, 2024 07:26
@xiaoxiang781216
Copy link
Contributor

let's close this pr since the fix is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Arch: ceva Issues related to CEVA architecture Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants