You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nuttx.apache.org by Jukka Laitinen <ju...@iki.fi> on 2022/03/25 07:35:00 UTC
NuttX github code review practices
Hi,
Please don't take the following as a rant, but rather as a kind reminder
for people conducting code reviews for NuttX.
Please, respect the author. You don't need to re-write every line of the
patch in comments just because you feel that the variable name could be
different or something is not defined in alphabetical order. The author
may have some other reason to e.g. order variable definitions in the
source files. NuttX has it's coding standard (and nxstyle) as well as
inviolables which must be obeyed. Anything past to that goes easily too far.
Again, please respect the author. There are often different ways of
achieving the same goal or functionality. And it is fine to challenge
and discuss of the alternatives. But if the author explains why he has
ended up with a certain implementation, there is no need to challenge
that endlessly, if it doesn't break things for others.
I am bringing this up, since I have seen many PR's which are important
for me and the organizations which I work for, getting stuck in endless
debate and nitpicking.
Just some time ago, a single ethernet driver, was stuck for a week for
style issues (and the style was complying with NuttX coding standards in
the first place).
As an another example, we would very much like to bring in
CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard
on this for some time, and have it working. Now, even when this work it
is only additions, not breaking anything for FLAT_BUILD users, is stuck
in endless debate where half of the comments show that the reviewer
doesn't know the RISC-V ISA thoroughly, and is referring to how things
are done in ARM world. And the other half is largely useless style
nitpicking.
Again, it is fine to discuss, give comments and suggest alternatives -
just respect and trust the author when he explains why he has taken some
paths in the implementation.
The current ways of inspecting new functionality gets on my nerves, as
at the same time we are getting build script changes and "code
harmonizing" changes with fast pace into mainline, which completely
break things for my own platforms. That is, large changes which for
example touch every board and are not tested on those. I am myself
spending at least a day every week digging through these "what is broken
this time".
Finally, I truly respect and appreciate that people put their time into
reviewing all the stuff! So please, please don't take the above as an
"attack" against anyone. I just want to remind everyone about respect
for the authors.
Thanks,
Jukka
Re: NuttX github code review practices
Posted by Xiang Xiao <xi...@gmail.com>.
On Mon, Mar 28, 2022 at 5:30 PM Jukka Laitinen <ju...@iki.fi>
wrote:
> Another example:
>
> https://github.com/apache/incubator-nuttx/pull/5731
>
> A PR which looks like 0 gain, 100 risk of breaking everything (breaks
> our stuff at least),
This patch can improve the context switch performance, since it could avoid
the follow memory copy in every context switch:
1. 32 integer register(128B on 32bit, 256B on 64bit)
2. 32 float register(128B single precision, 256B double precision)
3. Two status register
And at the same time, each tcb can also reduce the same amount of memory.
So, could you give more info why this change is zero gain?
> and can only work in FLAT_BUILD mode (if even in
> that?).
>
This patch should work fine for both flat and protected builds, both Xiaomi
and Sony test on several boards.
>
> How does stuff like this get in? Xiaomi does and approves by themselves?
> Sorry for being blunt, but this is really irritating.
>
>
This patch is developed by Xiaomi, but reviewed, tested and merged by Sony.
Normally, big changes like this will be reviewed by different groups and
will be held at least several days before merging.
Anyone could give the feedback on github and all comments must be addressed
before merging. If you look at this patch closely:
1. This patch was created 15 days ago
2. And has more than 20 comments
> -Jukka
>
>
> On 25.3.2022 21.47, Xiang Xiao wrote:
> > On Fri, Mar 25, 2022 at 6:53 PM Jukka Laitinen <ju...@iki.fi>
> > wrote:
> >
> >> Hi,
> >>
> >> I was trying to make a more general statement than starting discussion
> >> on separate PRs, but let me shortly answer still
> >>
> >> On 25.3.2022 10.32, Xiang Xiao wrote:
> >>> On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <ju...@iki.fi>
> >>> wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>>
> >>>> As an another example, we would very much like to bring in
> >>>> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked
> hard
> >>>> on this for some time, and have it working. Now, even when this work
> it
> >>>> is only additions, not breaking anything for FLAT_BUILD users,
> >>> Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or
> code
> >> is
> >>> perfect, that's why the interesting people can give the comment.
> >> But of course!
> >>>> is stuck
> >>>> in endless debate where half of the comments show that the reviewer
> >>>> doesn't know the RISC-V ISA thoroughly,
> >>>>
> >>>>
> >>>> Sorry, I give you this impression, but I have worked on RiSCV since
> >> 2018:(.
> >>
> >> I wasn't referring to you with the above comment... and I am sorry about
> >> this. I'll try to avoid anything that can be interpreted as comments to
> >> person, in the future.
> >>
> >>>> and is referring to how things
> >>>> are done in ARM world. And the other half is largely useless style
> >>>> nitpicking.
> >>>>
> >>> I think the most debate is about how S-mode talks with M-mode in this
> PR.
> >>> The module design and standard compliant is always NuttX core value and
> >>> highlight in:
> >>> https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
> >>> Since OpenSBI is adopted by many OS to interact with M-mode firmware:
> >>> https://github.com/riscv-software-src/opensbi
> >>> It's always good to follow the best practice and what I insist in this
> >> PR:
> >>> we can implement a minimal subset of OpenSBI to support S-mode NuttX,
> >> We (TII, it's research partners and subcontractors) will not be using
> >> OpenSBI for interfacing between S-mode and M-mode inside NuttX, since it
> >> doesn't make any sense for us. For running NuttX in kernel mode, we will
> >> not "implement a subset of OpenSBI". We (Ville) have implemented the
> >> "native SBI for NuttX". OpenSBI is not a standard, it is just a library
> >> maintained by a few people. It is much better to just do what is needed
> >> to do what is good for NuttX, following the standard RISC-V ISA spec,
> >> and not blindly go with a 3rd party bloated library.
> >>
> > Ok, if you still prefer native SBI, please at least add a choice option
> in
> > Kconfig,
> > so other people can support OpenSBI later.
> >
> >
> >>> but
> >>> don't invent a private interface since M-mode firmware mayn't run NuttX
> >> at
> >>> all.
> >> Now this is fair request, and I don't see any problems in implementing
> >> this also for the mpfs in the future. Currently that PR (IMHO) doesn't
> >> prevent having another M-mode firmware+any custom SBI such as OpenSBI on
> >> other platforms. Currently there are also parts under mpfs board, which
> >> can be generalized later on to avoid code copy paste, if others want to
> >> use our implementation on other risc-v archs. Right now there is no
> >> copy-paste code added, since this is not used in any other risc-v archs.
> >>
> >> I see bringing in CONFIG_BUILD_KERNEL as a very important step, and I
> >> see it a huge step forward if we can get in even one new architecture in
> >> supporting that. But putting too much pressure on making world perfect
> >> for the whole risc-v/nuttx community at once is just too much.
> >> Incremental steps forward is much better way... First make it work on
> >> one board, then generalize functionality when taking it into use in
> >> another and so on...
> >>
> > Sure, but since it's a bigger change and the implementation is unique in
> > many places, it's expected that the review process will be a bit longer.
> >
> >
> >> Anyhow, let's try to keep the discussion regarding a single PR within
> >> that PR in github. And I am really sorry if you felt that I was accusing
> >> you of something. it was not my intention. I truly appreciate that you
> >> are putting so much time and effort for reviewing patches to NuttX.
> >>
> >> Thanks,
> >>
> >> Jukka
> >>
> >>
> >>
>
Re: NuttX github code review practices
Posted by Xiang Xiao <xi...@gmail.com>.
On Mon, Mar 28, 2022 at 6:02 PM Sebastien Lorquet <se...@lorquet.fr>
wrote:
> In this example it's Xiaomi and Sony.
>
> NuttX has a code review problem and it has to be identified and addressed.
>
The review strictly follows the process setup and agreed by the community,
and the process is also similar to other Apache projects.
But anyone could provide a better suggestion or workflow to improve the
process.
>
> I have the same feeling here, last time I tried to send a pull request,
> it took several day to fix style issues for a ONE LINE code typo.
>
Could you point out the PR? Most minor changes will be reviewed within one
day I think.
>
> And a lot of board breaking changes are committed regularly. when you
> have a custom board it's not fun.
>
Sorry for the inconvenience. But, sometime i
> I believe a LOT of things including new features happens "silently" in
> github comments only,
Yes, that's because the agreed workflow is based on github.
> and very little is discussed on this mailing list.
>
>
Do you like the email based workflow like the Linux community? Since it's a
fundamental change, it's better to start a new thread to discuss.
> Sebastien
>
>
> Le 28/03/2022 à 11:30, Jukka Laitinen a écrit :
> > Another example:
> >
> > https://github.com/apache/incubator-nuttx/pull/5731
> >
> > A PR which looks like 0 gain, 100 risk of breaking everything (breaks
> > our stuff at least), and can only work in FLAT_BUILD mode (if even in
> > that?).
> >
> > How does stuff like this get in? Xiaomi does and approves by
> > themselves? Sorry for being blunt, but this is really irritating.
> >
> > -Jukka
> >
> >
> > On 25.3.2022 21.47, Xiang Xiao wrote:
> >> On Fri, Mar 25, 2022 at 6:53 PM Jukka Laitinen <ju...@iki.fi>
> >> wrote:
> >>
> >>> Hi,
> >>>
> >>> I was trying to make a more general statement than starting discussion
> >>> on separate PRs, but let me shortly answer still
> >>>
> >>> On 25.3.2022 10.32, Xiang Xiao wrote:
> >>>> On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <jukka.laitinen@iki.fi
> >
> >>>> wrote:
> >>>>
> >>>>> Hi,
> >>>>>
> >>>>>
> >>>>> As an another example, we would very much like to bring in
> >>>>> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have
> >>>>> worked hard
> >>>>> on this for some time, and have it working. Now, even when this
> >>>>> work it
> >>>>> is only additions, not breaking anything for FLAT_BUILD users,
> >>>> Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or
> >>>> code
> >>> is
> >>>> perfect, that's why the interesting people can give the comment.
> >>> But of course!
> >>>>> is stuck
> >>>>> in endless debate where half of the comments show that the reviewer
> >>>>> doesn't know the RISC-V ISA thoroughly,
> >>>>>
> >>>>>
> >>>>> Sorry, I give you this impression, but I have worked on RiSCV since
> >>> 2018:(.
> >>>
> >>> I wasn't referring to you with the above comment... and I am sorry
> >>> about
> >>> this. I'll try to avoid anything that can be interpreted as comments to
> >>> person, in the future.
> >>>
> >>>>> and is referring to how things
> >>>>> are done in ARM world. And the other half is largely useless style
> >>>>> nitpicking.
> >>>>>
> >>>> I think the most debate is about how S-mode talks with M-mode in
> >>>> this PR.
> >>>> The module design and standard compliant is always NuttX core value
> >>>> and
> >>>> highlight in:
> >>>> https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
> >>>> Since OpenSBI is adopted by many OS to interact with M-mode firmware:
> >>>> https://github.com/riscv-software-src/opensbi
> >>>> It's always good to follow the best practice and what I insist in this
> >>> PR:
> >>>> we can implement a minimal subset of OpenSBI to support S-mode NuttX,
> >>> We (TII, it's research partners and subcontractors) will not be using
> >>> OpenSBI for interfacing between S-mode and M-mode inside NuttX,
> >>> since it
> >>> doesn't make any sense for us. For running NuttX in kernel mode, we
> >>> will
> >>> not "implement a subset of OpenSBI". We (Ville) have implemented the
> >>> "native SBI for NuttX". OpenSBI is not a standard, it is just a library
> >>> maintained by a few people. It is much better to just do what is needed
> >>> to do what is good for NuttX, following the standard RISC-V ISA spec,
> >>> and not blindly go with a 3rd party bloated library.
> >>>
> >> Ok, if you still prefer native SBI, please at least add a choice
> >> option in
> >> Kconfig,
> >> so other people can support OpenSBI later.
> >>
> >>
> >>>> but
> >>>> don't invent a private interface since M-mode firmware mayn't run
> >>>> NuttX
> >>> at
> >>>> all.
> >>> Now this is fair request, and I don't see any problems in implementing
> >>> this also for the mpfs in the future. Currently that PR (IMHO) doesn't
> >>> prevent having another M-mode firmware+any custom SBI such as
> >>> OpenSBI on
> >>> other platforms. Currently there are also parts under mpfs board, which
> >>> can be generalized later on to avoid code copy paste, if others want to
> >>> use our implementation on other risc-v archs. Right now there is no
> >>> copy-paste code added, since this is not used in any other risc-v
> >>> archs.
> >>>
> >>> I see bringing in CONFIG_BUILD_KERNEL as a very important step, and I
> >>> see it a huge step forward if we can get in even one new
> >>> architecture in
> >>> supporting that. But putting too much pressure on making world perfect
> >>> for the whole risc-v/nuttx community at once is just too much.
> >>> Incremental steps forward is much better way... First make it work on
> >>> one board, then generalize functionality when taking it into use in
> >>> another and so on...
> >>>
> >> Sure, but since it's a bigger change and the implementation is unique in
> >> many places, it's expected that the review process will be a bit longer.
> >>
> >>
> >>> Anyhow, let's try to keep the discussion regarding a single PR within
> >>> that PR in github. And I am really sorry if you felt that I was
> >>> accusing
> >>> you of something. it was not my intention. I truly appreciate that you
> >>> are putting so much time and effort for reviewing patches to NuttX.
> >>>
> >>> Thanks,
> >>>
> >>> Jukka
> >>>
> >>>
> >>>
>
Re: NuttX github code review practices
Posted by Abdelatif Guettouche <ab...@gmail.com>.
> Have a few custom boards in the test process. we're talking build
> testing, not runtime. No need for a farm.
We actually should do this. It should be possible to have a few
custom boards in another repository (the old testing repo for
instance) and trigger its workflow on PRs.
I believe once Greg suggested to use a special sim config with the
necessary custom options enabled, if that work that would be simpler.
However, I'm not sure if we can test all the possible configurations
like this, for example we have boards with common folder, boards with
no common folder and boards with common but the user doesn't want the
common folder. Either way, we should do something about custom boards
to avoid any breakage.
Regarding testing in hardware, what we have right now is different
organisations doing their own tests on their own infrastructure. In
most cases, this means that we wait until the change is already in
master. For us (as in Espressif) we sometimes push branches
internally with the changes of some PRs that we think require more
tests.
I don't know how we can setup hardware test runners with Apache
infrastructure, the best thing we can do is to have QEMU for most
chips.
This is a bit out of topic, sorry about this Jukka, we can move this
discussion to a different thread if you want.
Regarding code review, please remember that _anyone_ in the community
can participate, it doesn't have to be a commiter or PPMC member. If
a concern is raised by anyone, the corresponding PR will have to wait
until everything is addressed.
So if you guys have some spare time, you can check PRs of interest,
you don't have to go through all the commits of all the PRs, any help
is appreciated.
On Mon, Mar 28, 2022 at 2:51 PM Sebastien Lorquet <se...@lorquet.fr> wrote:
>
> Sorry, I cant possibly test every commit and tell you what breaks
>
> Have a few custom boards in the test process. we're talking build
> testing, not runtime. No need for a farm.
>
> I am not actively developing in nuttx. I'm just using it.
>
>
> Your request is ONLY possible for contributors working on NuttX full
> time on the very long term.
>
> It's not my situation. Following the github feeds would result in
> massive amounts of commit "spam" to read, sort, review, delete. sorry
> not possible given the amount of commits by certains companies and
> various topics.
>
> But still I see the project evolving with fear and tbh, because at some
> point I'll have to work on a new board (it's planned).
>
>
> I am not here to rant more. These were feelings and of course it's
> biased. I was just adding my voice to Jukka's concerns.
>
>
> Sebastien
>
>
>
>
> Le 28/03/2022 à 14:03, Alan Carvalho de Assis a écrit :
> > On 3/28/22, Sebastien Lorquet <se...@lorquet.fr> wrote:
> >> In this example it's Xiaomi and Sony.
> >>
> >> NuttX has a code review problem and it has to be identified and addressed.
> >>
> >> I have the same feeling here, last time I tried to send a pull request,
> >> it took several day to fix style issues for a ONE LINE code typo.
> >>
> > Most probably you didn't follow the process to verify for code style errors etc:
> > https://nuttx.apache.org/docs/latest/contributing/making-changes.html#git-workflow-with-an-upstream-repository
> >
> >> And a lot of board breaking changes are committed regularly. when you
> >> have a custom board it's not fun.
> >>
> > Please help us to test! We don't have CI with a board farm test to help us.
> >
> >> I believe a LOT of things including new features happens "silently" in
> >> github comments only, and very little is discussed on this mailing list.
> >>
> > This mailing list purpose it not for code reviewing, but you can
> > subscribe on github or apache repository to receive an email with each
> > PR and with each comment other people are doing.
> >
> > We need more people to review the code, not more people to complain!
> >
> > BR,
> >
> > Alan
Re: NuttX github code review practices
Posted by Sebastien Lorquet <se...@lorquet.fr>.
Sorry, I cant possibly test every commit and tell you what breaks
Have a few custom boards in the test process. we're talking build
testing, not runtime. No need for a farm.
I am not actively developing in nuttx. I'm just using it.
Your request is ONLY possible for contributors working on NuttX full
time on the very long term.
It's not my situation. Following the github feeds would result in
massive amounts of commit "spam" to read, sort, review, delete. sorry
not possible given the amount of commits by certains companies and
various topics.
But still I see the project evolving with fear and tbh, because at some
point I'll have to work on a new board (it's planned).
I am not here to rant more. These were feelings and of course it's
biased. I was just adding my voice to Jukka's concerns.
Sebastien
Le 28/03/2022 à 14:03, Alan Carvalho de Assis a écrit :
> On 3/28/22, Sebastien Lorquet <se...@lorquet.fr> wrote:
>> In this example it's Xiaomi and Sony.
>>
>> NuttX has a code review problem and it has to be identified and addressed.
>>
>> I have the same feeling here, last time I tried to send a pull request,
>> it took several day to fix style issues for a ONE LINE code typo.
>>
> Most probably you didn't follow the process to verify for code style errors etc:
> https://nuttx.apache.org/docs/latest/contributing/making-changes.html#git-workflow-with-an-upstream-repository
>
>> And a lot of board breaking changes are committed regularly. when you
>> have a custom board it's not fun.
>>
> Please help us to test! We don't have CI with a board farm test to help us.
>
>> I believe a LOT of things including new features happens "silently" in
>> github comments only, and very little is discussed on this mailing list.
>>
> This mailing list purpose it not for code reviewing, but you can
> subscribe on github or apache repository to receive an email with each
> PR and with each comment other people are doing.
>
> We need more people to review the code, not more people to complain!
>
> BR,
>
> Alan
Re: NuttX github code review practices
Posted by Alan Carvalho de Assis <ac...@gmail.com>.
On 3/28/22, Sebastien Lorquet <se...@lorquet.fr> wrote:
> In this example it's Xiaomi and Sony.
>
> NuttX has a code review problem and it has to be identified and addressed.
>
> I have the same feeling here, last time I tried to send a pull request,
> it took several day to fix style issues for a ONE LINE code typo.
>
Most probably you didn't follow the process to verify for code style errors etc:
https://nuttx.apache.org/docs/latest/contributing/making-changes.html#git-workflow-with-an-upstream-repository
> And a lot of board breaking changes are committed regularly. when you
> have a custom board it's not fun.
>
Please help us to test! We don't have CI with a board farm test to help us.
> I believe a LOT of things including new features happens "silently" in
> github comments only, and very little is discussed on this mailing list.
>
This mailing list purpose it not for code reviewing, but you can
subscribe on github or apache repository to receive an email with each
PR and with each comment other people are doing.
We need more people to review the code, not more people to complain!
BR,
Alan
Re: NuttX github code review practices
Posted by Sebastien Lorquet <se...@lorquet.fr>.
In this example it's Xiaomi and Sony.
NuttX has a code review problem and it has to be identified and addressed.
I have the same feeling here, last time I tried to send a pull request,
it took several day to fix style issues for a ONE LINE code typo.
And a lot of board breaking changes are committed regularly. when you
have a custom board it's not fun.
I believe a LOT of things including new features happens "silently" in
github comments only, and very little is discussed on this mailing list.
Sebastien
Le 28/03/2022 à 11:30, Jukka Laitinen a écrit :
> Another example:
>
> https://github.com/apache/incubator-nuttx/pull/5731
>
> A PR which looks like 0 gain, 100 risk of breaking everything (breaks
> our stuff at least), and can only work in FLAT_BUILD mode (if even in
> that?).
>
> How does stuff like this get in? Xiaomi does and approves by
> themselves? Sorry for being blunt, but this is really irritating.
>
> -Jukka
>
>
> On 25.3.2022 21.47, Xiang Xiao wrote:
>> On Fri, Mar 25, 2022 at 6:53 PM Jukka Laitinen <ju...@iki.fi>
>> wrote:
>>
>>> Hi,
>>>
>>> I was trying to make a more general statement than starting discussion
>>> on separate PRs, but let me shortly answer still
>>>
>>> On 25.3.2022 10.32, Xiang Xiao wrote:
>>>> On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <ju...@iki.fi>
>>>> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> As an another example, we would very much like to bring in
>>>>> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have
>>>>> worked hard
>>>>> on this for some time, and have it working. Now, even when this
>>>>> work it
>>>>> is only additions, not breaking anything for FLAT_BUILD users,
>>>> Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or
>>>> code
>>> is
>>>> perfect, that's why the interesting people can give the comment.
>>> But of course!
>>>>> is stuck
>>>>> in endless debate where half of the comments show that the reviewer
>>>>> doesn't know the RISC-V ISA thoroughly,
>>>>>
>>>>>
>>>>> Sorry, I give you this impression, but I have worked on RiSCV since
>>> 2018:(.
>>>
>>> I wasn't referring to you with the above comment... and I am sorry
>>> about
>>> this. I'll try to avoid anything that can be interpreted as comments to
>>> person, in the future.
>>>
>>>>> and is referring to how things
>>>>> are done in ARM world. And the other half is largely useless style
>>>>> nitpicking.
>>>>>
>>>> I think the most debate is about how S-mode talks with M-mode in
>>>> this PR.
>>>> The module design and standard compliant is always NuttX core value
>>>> and
>>>> highlight in:
>>>> https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
>>>> Since OpenSBI is adopted by many OS to interact with M-mode firmware:
>>>> https://github.com/riscv-software-src/opensbi
>>>> It's always good to follow the best practice and what I insist in this
>>> PR:
>>>> we can implement a minimal subset of OpenSBI to support S-mode NuttX,
>>> We (TII, it's research partners and subcontractors) will not be using
>>> OpenSBI for interfacing between S-mode and M-mode inside NuttX,
>>> since it
>>> doesn't make any sense for us. For running NuttX in kernel mode, we
>>> will
>>> not "implement a subset of OpenSBI". We (Ville) have implemented the
>>> "native SBI for NuttX". OpenSBI is not a standard, it is just a library
>>> maintained by a few people. It is much better to just do what is needed
>>> to do what is good for NuttX, following the standard RISC-V ISA spec,
>>> and not blindly go with a 3rd party bloated library.
>>>
>> Ok, if you still prefer native SBI, please at least add a choice
>> option in
>> Kconfig,
>> so other people can support OpenSBI later.
>>
>>
>>>> but
>>>> don't invent a private interface since M-mode firmware mayn't run
>>>> NuttX
>>> at
>>>> all.
>>> Now this is fair request, and I don't see any problems in implementing
>>> this also for the mpfs in the future. Currently that PR (IMHO) doesn't
>>> prevent having another M-mode firmware+any custom SBI such as
>>> OpenSBI on
>>> other platforms. Currently there are also parts under mpfs board, which
>>> can be generalized later on to avoid code copy paste, if others want to
>>> use our implementation on other risc-v archs. Right now there is no
>>> copy-paste code added, since this is not used in any other risc-v
>>> archs.
>>>
>>> I see bringing in CONFIG_BUILD_KERNEL as a very important step, and I
>>> see it a huge step forward if we can get in even one new
>>> architecture in
>>> supporting that. But putting too much pressure on making world perfect
>>> for the whole risc-v/nuttx community at once is just too much.
>>> Incremental steps forward is much better way... First make it work on
>>> one board, then generalize functionality when taking it into use in
>>> another and so on...
>>>
>> Sure, but since it's a bigger change and the implementation is unique in
>> many places, it's expected that the review process will be a bit longer.
>>
>>
>>> Anyhow, let's try to keep the discussion regarding a single PR within
>>> that PR in github. And I am really sorry if you felt that I was
>>> accusing
>>> you of something. it was not my intention. I truly appreciate that you
>>> are putting so much time and effort for reviewing patches to NuttX.
>>>
>>> Thanks,
>>>
>>> Jukka
>>>
>>>
>>>
Re: NuttX github code review practices
Posted by Jukka Laitinen <ju...@iki.fi>.
Another example:
https://github.com/apache/incubator-nuttx/pull/5731
A PR which looks like 0 gain, 100 risk of breaking everything (breaks
our stuff at least), and can only work in FLAT_BUILD mode (if even in
that?).
How does stuff like this get in? Xiaomi does and approves by themselves?
Sorry for being blunt, but this is really irritating.
-Jukka
On 25.3.2022 21.47, Xiang Xiao wrote:
> On Fri, Mar 25, 2022 at 6:53 PM Jukka Laitinen <ju...@iki.fi>
> wrote:
>
>> Hi,
>>
>> I was trying to make a more general statement than starting discussion
>> on separate PRs, but let me shortly answer still
>>
>> On 25.3.2022 10.32, Xiang Xiao wrote:
>>> On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <ju...@iki.fi>
>>> wrote:
>>>
>>>> Hi,
>>>>
>>>>
>>>> As an another example, we would very much like to bring in
>>>> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard
>>>> on this for some time, and have it working. Now, even when this work it
>>>> is only additions, not breaking anything for FLAT_BUILD users,
>>> Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or code
>> is
>>> perfect, that's why the interesting people can give the comment.
>> But of course!
>>>> is stuck
>>>> in endless debate where half of the comments show that the reviewer
>>>> doesn't know the RISC-V ISA thoroughly,
>>>>
>>>>
>>>> Sorry, I give you this impression, but I have worked on RiSCV since
>> 2018:(.
>>
>> I wasn't referring to you with the above comment... and I am sorry about
>> this. I'll try to avoid anything that can be interpreted as comments to
>> person, in the future.
>>
>>>> and is referring to how things
>>>> are done in ARM world. And the other half is largely useless style
>>>> nitpicking.
>>>>
>>> I think the most debate is about how S-mode talks with M-mode in this PR.
>>> The module design and standard compliant is always NuttX core value and
>>> highlight in:
>>> https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
>>> Since OpenSBI is adopted by many OS to interact with M-mode firmware:
>>> https://github.com/riscv-software-src/opensbi
>>> It's always good to follow the best practice and what I insist in this
>> PR:
>>> we can implement a minimal subset of OpenSBI to support S-mode NuttX,
>> We (TII, it's research partners and subcontractors) will not be using
>> OpenSBI for interfacing between S-mode and M-mode inside NuttX, since it
>> doesn't make any sense for us. For running NuttX in kernel mode, we will
>> not "implement a subset of OpenSBI". We (Ville) have implemented the
>> "native SBI for NuttX". OpenSBI is not a standard, it is just a library
>> maintained by a few people. It is much better to just do what is needed
>> to do what is good for NuttX, following the standard RISC-V ISA spec,
>> and not blindly go with a 3rd party bloated library.
>>
> Ok, if you still prefer native SBI, please at least add a choice option in
> Kconfig,
> so other people can support OpenSBI later.
>
>
>>> but
>>> don't invent a private interface since M-mode firmware mayn't run NuttX
>> at
>>> all.
>> Now this is fair request, and I don't see any problems in implementing
>> this also for the mpfs in the future. Currently that PR (IMHO) doesn't
>> prevent having another M-mode firmware+any custom SBI such as OpenSBI on
>> other platforms. Currently there are also parts under mpfs board, which
>> can be generalized later on to avoid code copy paste, if others want to
>> use our implementation on other risc-v archs. Right now there is no
>> copy-paste code added, since this is not used in any other risc-v archs.
>>
>> I see bringing in CONFIG_BUILD_KERNEL as a very important step, and I
>> see it a huge step forward if we can get in even one new architecture in
>> supporting that. But putting too much pressure on making world perfect
>> for the whole risc-v/nuttx community at once is just too much.
>> Incremental steps forward is much better way... First make it work on
>> one board, then generalize functionality when taking it into use in
>> another and so on...
>>
> Sure, but since it's a bigger change and the implementation is unique in
> many places, it's expected that the review process will be a bit longer.
>
>
>> Anyhow, let's try to keep the discussion regarding a single PR within
>> that PR in github. And I am really sorry if you felt that I was accusing
>> you of something. it was not my intention. I truly appreciate that you
>> are putting so much time and effort for reviewing patches to NuttX.
>>
>> Thanks,
>>
>> Jukka
>>
>>
>>
Re: NuttX github code review practices
Posted by Xiang Xiao <xi...@gmail.com>.
On Fri, Mar 25, 2022 at 6:53 PM Jukka Laitinen <ju...@iki.fi>
wrote:
> Hi,
>
> I was trying to make a more general statement than starting discussion
> on separate PRs, but let me shortly answer still
>
> On 25.3.2022 10.32, Xiang Xiao wrote:
> > On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <ju...@iki.fi>
> > wrote:
> >
> >> Hi,
> >>
> >>
> >> As an another example, we would very much like to bring in
> >> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard
> >> on this for some time, and have it working. Now, even when this work it
> >> is only additions, not breaking anything for FLAT_BUILD users,
> >
> > Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or code
> is
> > perfect, that's why the interesting people can give the comment.
> But of course!
> >> is stuck
> >> in endless debate where half of the comments show that the reviewer
> >> doesn't know the RISC-V ISA thoroughly,
> >>
> >>
> >> Sorry, I give you this impression, but I have worked on RiSCV since
> 2018:(.
>
> I wasn't referring to you with the above comment... and I am sorry about
> this. I'll try to avoid anything that can be interpreted as comments to
> person, in the future.
>
> >> and is referring to how things
> >> are done in ARM world. And the other half is largely useless style
> >> nitpicking.
> >>
> > I think the most debate is about how S-mode talks with M-mode in this PR.
> > The module design and standard compliant is always NuttX core value and
> > highlight in:
> > https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
> > Since OpenSBI is adopted by many OS to interact with M-mode firmware:
> > https://github.com/riscv-software-src/opensbi
> > It's always good to follow the best practice and what I insist in this
> PR:
> > we can implement a minimal subset of OpenSBI to support S-mode NuttX,
>
> We (TII, it's research partners and subcontractors) will not be using
> OpenSBI for interfacing between S-mode and M-mode inside NuttX, since it
> doesn't make any sense for us. For running NuttX in kernel mode, we will
> not "implement a subset of OpenSBI". We (Ville) have implemented the
> "native SBI for NuttX". OpenSBI is not a standard, it is just a library
> maintained by a few people. It is much better to just do what is needed
> to do what is good for NuttX, following the standard RISC-V ISA spec,
> and not blindly go with a 3rd party bloated library.
>
Ok, if you still prefer native SBI, please at least add a choice option in
Kconfig,
so other people can support OpenSBI later.
>
> > but
> > don't invent a private interface since M-mode firmware mayn't run NuttX
> at
> > all.
>
> Now this is fair request, and I don't see any problems in implementing
> this also for the mpfs in the future. Currently that PR (IMHO) doesn't
> prevent having another M-mode firmware+any custom SBI such as OpenSBI on
> other platforms. Currently there are also parts under mpfs board, which
> can be generalized later on to avoid code copy paste, if others want to
> use our implementation on other risc-v archs. Right now there is no
> copy-paste code added, since this is not used in any other risc-v archs.
>
> I see bringing in CONFIG_BUILD_KERNEL as a very important step, and I
> see it a huge step forward if we can get in even one new architecture in
> supporting that. But putting too much pressure on making world perfect
> for the whole risc-v/nuttx community at once is just too much.
> Incremental steps forward is much better way... First make it work on
> one board, then generalize functionality when taking it into use in
> another and so on...
>
Sure, but since it's a bigger change and the implementation is unique in
many places, it's expected that the review process will be a bit longer.
>
> Anyhow, let's try to keep the discussion regarding a single PR within
> that PR in github. And I am really sorry if you felt that I was accusing
> you of something. it was not my intention. I truly appreciate that you
> are putting so much time and effort for reviewing patches to NuttX.
>
> Thanks,
>
> Jukka
>
>
>
Re: NuttX github code review practices
Posted by Jukka Laitinen <ju...@iki.fi>.
Hi,
I was trying to make a more general statement than starting discussion
on separate PRs, but let me shortly answer still
On 25.3.2022 10.32, Xiang Xiao wrote:
> On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <ju...@iki.fi>
> wrote:
>
>> Hi,
>>
>>
>> As an another example, we would very much like to bring in
>> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard
>> on this for some time, and have it working. Now, even when this work it
>> is only additions, not breaking anything for FLAT_BUILD users,
>
> Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or code is
> perfect, that's why the interesting people can give the comment.
But of course!
>> is stuck
>> in endless debate where half of the comments show that the reviewer
>> doesn't know the RISC-V ISA thoroughly,
>>
>>
>> Sorry, I give you this impression, but I have worked on RiSCV since 2018:(.
I wasn't referring to you with the above comment... and I am sorry about
this. I'll try to avoid anything that can be interpreted as comments to
person, in the future.
>> and is referring to how things
>> are done in ARM world. And the other half is largely useless style
>> nitpicking.
>>
> I think the most debate is about how S-mode talks with M-mode in this PR.
> The module design and standard compliant is always NuttX core value and
> highlight in:
> https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
> Since OpenSBI is adopted by many OS to interact with M-mode firmware:
> https://github.com/riscv-software-src/opensbi
> It's always good to follow the best practice and what I insist in this PR:
> we can implement a minimal subset of OpenSBI to support S-mode NuttX,
We (TII, it's research partners and subcontractors) will not be using
OpenSBI for interfacing between S-mode and M-mode inside NuttX, since it
doesn't make any sense for us. For running NuttX in kernel mode, we will
not "implement a subset of OpenSBI". We (Ville) have implemented the
"native SBI for NuttX". OpenSBI is not a standard, it is just a library
maintained by a few people. It is much better to just do what is needed
to do what is good for NuttX, following the standard RISC-V ISA spec,
and not blindly go with a 3rd party bloated library.
> but
> don't invent a private interface since M-mode firmware mayn't run NuttX at
> all.
Now this is fair request, and I don't see any problems in implementing
this also for the mpfs in the future. Currently that PR (IMHO) doesn't
prevent having another M-mode firmware+any custom SBI such as OpenSBI on
other platforms. Currently there are also parts under mpfs board, which
can be generalized later on to avoid code copy paste, if others want to
use our implementation on other risc-v archs. Right now there is no
copy-paste code added, since this is not used in any other risc-v archs.
I see bringing in CONFIG_BUILD_KERNEL as a very important step, and I
see it a huge step forward if we can get in even one new architecture in
supporting that. But putting too much pressure on making world perfect
for the whole risc-v/nuttx community at once is just too much.
Incremental steps forward is much better way... First make it work on
one board, then generalize functionality when taking it into use in
another and so on...
Anyhow, let's try to keep the discussion regarding a single PR within
that PR in github. And I am really sorry if you felt that I was accusing
you of something. it was not my intention. I truly appreciate that you
are putting so much time and effort for reviewing patches to NuttX.
Thanks,
Jukka
Re: NuttX github code review practices
Posted by Petro Karashchenko <pe...@gmail.com>.
Hi,
As I'm the person who usually generates a lot of "style related" comments I
want to reply as well.
Since GitHub does not have a "severity" option for the option it is not too
convenient to distinguish between "optional" with "must have" comments. So
usually I use the "Comment" button instead of "Request changes" on GitHub.
With this I do not want to generate any delay in merging of the changes by
other reviewers if they consider my comments as "optional". With my
comments I just want to highlight the places that the author needs to pay
attention to and either rework that place of reply that this is an
intentional change and he/she insists on existing implementation or will
app in the next PR or any other type of reply. That is usually a result of
my experience as a reviewer because I do not usually have an option to
checkout the changes on a laptop and review those with a full scope, so
when I suggest to change "if (size)" to "if (size > 0)" that just I want to
make sure that author meant it intentionally. because sometimes it really
happens that "size" is a pointer and "if (size)" is a typo instead of "if
(*size)". I do not claim to be a single source of truth, but rather a
"friend giving advice". That is why the review process exists I think. The
other reason why I'm writing all these comments is because sometimes people
just want to make things faster and just copy/paste code even without
looking into it, so my comment is a highlight for the author to rethink the
changes and give a meaningful reply or more often to rework the code.
I do not want to offend anyone with my comments and I hope that I always
write those with respect to the author. What I read in "between the lines"
Jukka is that maybe you want to say that authors do not always have the
capacity to handle many comments? I as a reviewer do not know the
availability of the author and rely only on a reply in GitHub comments. I'm
fully fine to merge the changes without many changes that I propose. And
sometimes I iterate through the project by myself and fix dozens of style
issues that I find and I hope that if each new file added will have less of
those that I will not spend too much time later :)
Best regards,
Petro
пт, 25 бер. 2022 р. о 10:32 Xiang Xiao <xi...@gmail.com> пише:
> On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <ju...@iki.fi>
> wrote:
>
> > Hi,
> >
> >
> > As an another example, we would very much like to bring in
> > CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard
> > on this for some time, and have it working. Now, even when this work it
> > is only additions, not breaking anything for FLAT_BUILD users,
>
>
> Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or code is
> perfect, that's why the interesting people can give the comment.
>
>
> > is stuck
> > in endless debate where half of the comments show that the reviewer
> > doesn't know the RISC-V ISA thoroughly,
>
>
> Sorry, I give you this impression, but I have worked on RiSCV since 2018:(.
>
>
> > and is referring to how things
> > are done in ARM world. And the other half is largely useless style
> > nitpicking.
> >
>
> I think the most debate is about how S-mode talks with M-mode in this PR.
> The module design and standard compliant is always NuttX core value and
> highlight in:
> https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
> Since OpenSBI is adopted by many OS to interact with M-mode firmware:
> https://github.com/riscv-software-src/opensbi
> It's always good to follow the best practice and what I insist in this PR:
> we can implement a minimal subset of OpenSBI to support S-mode NuttX, but
> don't invent a private interface since M-mode firmware mayn't run NuttX at
> all.
>
Re: NuttX github code review practices
Posted by Xiang Xiao <xi...@gmail.com>.
On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <ju...@iki.fi>
wrote:
> Hi,
>
>
> As an another example, we would very much like to bring in
> CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard
> on this for some time, and have it working. Now, even when this work it
> is only additions, not breaking anything for FLAT_BUILD users,
Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or code is
perfect, that's why the interesting people can give the comment.
> is stuck
> in endless debate where half of the comments show that the reviewer
> doesn't know the RISC-V ISA thoroughly,
Sorry, I give you this impression, but I have worked on RiSCV since 2018:(.
> and is referring to how things
> are done in ARM world. And the other half is largely useless style
> nitpicking.
>
I think the most debate is about how S-mode talks with M-mode in this PR.
The module design and standard compliant is always NuttX core value and
highlight in:
https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md
Since OpenSBI is adopted by many OS to interact with M-mode firmware:
https://github.com/riscv-software-src/opensbi
It's always good to follow the best practice and what I insist in this PR:
we can implement a minimal subset of OpenSBI to support S-mode NuttX, but
don't invent a private interface since M-mode firmware mayn't run NuttX at
all.