apache / nuttx

Apache NuttX is a mature, real-time embedded operating system (RTOS)
https://nuttx.apache.org/
Apache License 2.0
2.74k stars 1.14k forks source link

Please manage your code submission and don't make changes without testing #12266

Closed sakumisu closed 5 months ago

sakumisu commented 5 months ago

When we use nuttx code in our projects, i found many problems.

All of these are my suggestions, a little rude but useful, thank you, guys.

anchao commented 5 months ago

I'm curious which company you are from? Bouffalo? Do you have any specific questions about the points you mentioned above?

acassis commented 5 months ago

@sakumisu although some points your raised need to be improved (i.e. we don't have functional test integrated with our CI), the root causes you defined are not true or correct.

Please enlighten me: which mainline are you submitting those patches, because I failed to find them here in this mainline: https://github.com/apache/nuttx/commits?author=sakumisu

So, before pointing fingers and blame existent contributors, please do your part and have a positive mind and action.

sakumisu commented 5 months ago

@sakumisu although some points your raised need to be improved (i.e. we don't have functional test integrated with our CI), the root causes you defined are not true or correct.

Please enlighten me: which mainline are you submitting those patches, because I failed to find them here in this mainline: https://github.com/apache/nuttx/commits?author=sakumisu

So, before pointing fingers and blame existent contributors, please do your part and have a positive mind and action.

  • no compatibility? Run ostest, LTS and other tools to confirm everything still compatible
  • no code reviews? Please start reviewing the code at github
  • low performance compilation? True, it cannot compile in IBM AT, AFAIK
  • don't push code before testing: this is the only valid argument you told and this is why we have the Testing request for each PR, so if you see someone not filling this field, just ask he/she to do so
  • don't push too many commits: it is better to have many logically separated commits than a single commit
  • no commercial value? there are many companies using it, so it should have commercial value. NuttX kernel is not different from Linux kernel, it is a moving target. All companies are welcome to join and help to improve it.

Thank you for your quickly answer. I was commissioned to do performance optimization in nuttx, so i do not push code for nuttx. The most question is about net and usb, others i do not care. I used old nuttx verion but does not support some features like tcp out of order, if we update code, it will work but some others do not work or performance is lower than before.But all these if i use lwip, nothing problem.

sakumisu commented 5 months ago

And when i search some codes, something is found, like this IOB_SECTION macro, did you think it should be CONFIG_IOB_SECTION? This is why i said no code review, you know, i care little about nuttx low level code, this is just a coincidence.

acassis commented 5 months ago

@sakumisu many people (Samsung inclusive) decide to use lwIP because they think the NuttX NET stack is slow, actually it is not. By default the performance is not so good because all default parameters are focused on board with low amount of memory. You need to modify the network parameters to get better performance.

acassis commented 5 months ago

Yes, I think you found a BUG:

@xiaoxiang781216 please take a look:

mm/iob/iob_initialize.c:#ifdef IOB_SECTION

It should be CONFIG_IOB_SECTION

Thank you very much @sakumisu !!!

sakumisu commented 5 months ago

@sakumisu many people (Samsung inclusive) decide to use lwIP because they think the NuttX NET stack is slow, actually it is not. By default the performance is not so good because all default parameters are focused on board with low amount of memory. You need to modify the network parameters to get better performance.

Yes, i did. Newer version can do better than before.Feel sorry that i study a little about nuttx net, i will continue to do.

sakumisu commented 5 months ago

Many commits in a mini version can make others feel worried and confused about whether it i s stable, many people do not want too many changes.They will use old version but newer can do better, we should cost time to convince them.

sakumisu commented 5 months ago

That's all, thank you sir for your answer again.

anchao commented 5 months ago

And when i search some codes, something is found, like this IOB_SECTION macro, did you think it should be CONFIG_IOB_SECTION? This is why i said no code review, you know, i care little about nuttx low level code, this is just a coincidence.

Have you been reading the code seriously? The definition of IOB_SECTION in the makefile file, https://github.com/apache/nuttx/blob/master/mm/iob/Make.defs#L43 You should send targeted issues to the community instead of using empty and slightly ignorant viewpoints, This is a friendly community, please correct your attitude.

sakumisu commented 5 months ago

And when i search some codes, something is found, like this IOB_SECTION macro, did you think it should be CONFIG_IOB_SECTION? This is why i said no code review, you know, i care little about nuttx low level code, this is just a coincidence.

Have you been reading the code seriously? The definition of IOB_SECTION in the makefile file, https://github.com/apache/nuttx/blob/master/mm/iob/Make.defs#L43 You should send targeted issues to the community instead of using empty and slightly ignorant viewpoints, This is a friendly community, please correct your attitude.

So you think this codestyle that you like and are proud of is excellent? You think that's what everyone's going to be paying attention to? Why can't it be simple?

anchao commented 5 months ago

So you think this codestyle that you like and are proud of is excellent? You think that's what everyone's going to be paying attention to? Why can't it be simple?

You can do it now, immediately, PLEASE! There is unnecessary to discuss further if you just complain!