You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/06/24 20:40:36 UTC
[GitHub] [incubator-nuttx-apps] patacongo opened a new pull request #307: Revert "nshlib: remove the dependency of date on RTC"
patacongo opened a new pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307
## Summary
This is a bad change. It has been show to cause an increase in size by around 2.3Kb in minimal configurations that cannot tolerate that massive size increase.
This reverts commit 4adb83c754500cfebe4c24a498eb4139e3ff8866.
## Impact
This will correct the behavior of the configuration bying restoring what is was in NuttX-9.0
## Testing
Size problem demonstrated on stm32f4discovery:nsh
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307#issuecomment-649094081
>
>
> I think the original intention of @anchao was really good because he tried to disable the 'date' command when DEFAULT_SMALL was defined, then it should reduces 3KB for those boards. However the side effect was that all boards which doesn't have DEFAULT_SMALL gained +3KB.
It is best to revert the PR. The origin conditions are the most correct. If RTC is enabled then the date command really must be supported. Otherwise, there is no way to manually set the RTC. If the RTC is not enabled, then the date command may occasionally still serve some limited purpose and the option to enable it still available.
Changing configurations by changing the default setting is almost always a very bad idea and should not in general be done. IF if is done then, there must also be an accompanying change to modify all affected defconfig files. Otherwise, you also change the behavior of every configuration. We cannot permit that in the future.
The best solution is to revert the PR. The original was correct, the modification is not. If there some reason to retain the modification (there is NOT), then there would also have to be a massive PR to change every defconfig file (except those few that set CONFIG_RTC). There are 600 defconfig files. NONE of them explicitly set CONFIG_NSHLIB_DISABLE_DATE. 35 of those set CONFIG_RTC=y and those all expect CONFIG_NSHLIB_DISABLE_DATE=n. 565 of them are currently broken because this PR: Those ALL expect CONFIG_NSHLIB_DISABLE_DATE=y. This PR screws them all.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307#issuecomment-650571924
>
>
> It is strange if we still want to use the 'date' command to get the synchronized time from NTP server ... Seriously, not all of the devices include RTC modules.
This is just the default setting. It does not prevent you from selecting the option. DO NOT CHANGE DEFAULT SETTINGS unless you also change all of the effected defconfig files. That breaks configurations.
Nothing is broken nothing should be fixed. Adding NTP as an condition would be acceptable, but changing the default is just wrong. Please do not do that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] btashton commented on pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307#issuecomment-649062272
@patacongo agree that we should revert this., thanks.
I wanted to tag @anchao since they made the original change to see if there is something else that needs to be done for the use-case this change was made for that does not impact the other builds.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307#issuecomment-649299806
@anchao and I talk about this before. The reason we made this change because CLOCK_REALTIME which date command really use doesn't depend on CONFIG_RTC in the current implementation, so it doesn't make sense to couple CONFIG_NSHLIB_DISABLE_DATE with CONFIG_RTC from the pure technology view, but the size explosion is the bad side effect we don't want for the small soc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] btashton merged pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
btashton merged pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] acassis commented on pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307#issuecomment-649073209
I think the original intention of @anchao was really good because he tried to disable the 'date' command when DEFAULT_SMALL was defined, then it should reduces 3KB for those boards. However the side effect was that all boards which doesn't have DEFAULT_SMALL gained +3KB.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo edited a comment on pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307#issuecomment-650571924
>
>
> It is strange if we still want to use the 'date' command to get the synchronized time from NTP server ... Seriously, not all of the devices include RTC modules.
This is just the default setting. It does not prevent you from selecting the option. DO NOT CHANGE DEFAULT SETTINGS unless you also change all of the effected defconfig files. That breaks configurations.
Nothing is broken nothing should be fixed. Adding NTP as an condition would be acceptable, but changing the default is just wrong. Please do not do that.
It is not wrong technically, it is wrong because it changes about 565 other configurations incorrectly. That is what must be avoided.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] anchao commented on pull request #307: Revert "nshlib: remove the dependency of date on RTC"
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #307:
URL: https://github.com/apache/incubator-nuttx-apps/pull/307#issuecomment-650570915
It is strange if we still want to use the 'date' command to get the synchronized time from NTP server ... Seriously, not all of the devices include RTC modules.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org