On Mon, Mar 28, 2022 at 6:02 PM Sebastien Lorquet <sebast...@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 <jukka.laiti...@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.laiti...@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 > >>> > >>> > >>> >
