Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sakumisu opened this issue Apr 30, 2024 · 12 comments
Closed

Comments

@sakumisu
Copy link

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

  • No compatibility(When we worked with old version, but failed with too many asserts or trap or strange apperance)
  • No codereview and no owners for some modules like net, usb and so on, anyone can push code and merged in, no speciality. We got much trouble and we need to fix by ourselves and update nuttx stream, it wastes our time.
  • Low performance compilation, you should know, many of us work in company and may have a low-performance computer, when we build nuttx, it costs our much time.
  • The most important is that please push code before you have tested and make sure the performance is fine.
  • Last, do not push too many commits in a mini release version, this gives the impression that the code is very unstable
  • These if you cannot do better, it can only be an opensource code but without commercial value.

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

@anchao
Copy link
Contributor

anchao commented Apr 30, 2024

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

@acassis
Copy link
Contributor

acassis commented Apr 30, 2024

@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 PC 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, then 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.

@sakumisu
Copy link
Author

@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
Copy link
Author

sakumisu commented Apr 30, 2024

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
Copy link
Contributor

acassis commented Apr 30, 2024

@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
Copy link
Contributor

acassis commented Apr 30, 2024

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Author

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

@anchao
Copy link
Contributor

anchao commented Apr 30, 2024

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
Copy link
Author

sakumisu commented May 1, 2024

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
Copy link
Contributor

anchao commented May 1, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants