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/02/22 09:41:35 UTC
[GitHub] [incubator-nuttx] klmchp opened a new issue #353: multiple
definition of `up_mdelay'
klmchp opened a new issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353
Hi ,
multiple definition of `up_mdelay' occurs when enabled CONFIG_ALAM_ARCH.
In nuttx/arch/xxxx/common/up_mdelay.c there is a implementation of up_mdelay.
But if follows the steps as below to enabled CONFIG_ALARM_ARCH:
make menuconfig
Device Drivers -->
Timer Driver Support -->
[*] Oneshot timer driver -->
[*] Alarm Arch Implementation // will enabled CONFIG_ALARM_ARCH.
in nuttx/drivers/timers/Make.defs
ifeq ($(CONFIG_ALARM_ARCH),y)
CSRCS += arch_alarm.c
TMRDEPPATH = --dep-path timers
TMRVPATH = :timers
endif
In arch_alarm.c, a up_delay also can be found here.
`void up_mdelay(unsigned int milliseconds)
{
up_udelay(USEC_PER_MSEC * milliseconds);
}`
BR
Kevin `Liu`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589976655
@klmchp and @patacongo , we don't need add any config here: if some chipset decide to use arch_timer.c, the chipset just need remove up_delay.c from it's Make.defs.
I added arch_timer.c/arch_rtc.c/arch_alarm.c just because NuttX define two similar timer/rtc inteface:
1.One set come from include/nuttx/arch.h which all start with up_
2.One set come from include/nuttx/timers/[oneshot.h|timer.h|rtc.h]
It doesn't make sense to let developer learn and implement two interface for one hardware.
Since the driver interface is bigger(multiple instance, alarm, /dev/xxx ...) than arch interface, it make sense to implement up_ timer/rtc API on top of driver interface, then the developer just need write timer/rtc/oneshot driver and let arch_timer.c/arch_rtc.c/arch_alarm.c do the conversion(remove up_delay.c from Make.defs since the implementation provide by arch_timer.c now).
Of course, the devloper can still implement up_xxx directly if they like.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590080225
> @patacongo @xiaoxiang781216 , also find multi definition of up_rtc_getdatetime, up_rtc_settime and so on. I list the code from nuttx/drivers/timers/arch_rtc.c for reference.
> `int up_rtc_getdatetime(FAR struct tm *tp)
> {
> if (g_rtc_lower != NULL)
> {
> struct rtc_time rtctime;
>
> ```
> ret = g_rtc_lower->ops->rdtime(g_rtc_lower, &rtctime);
> ```
>
> There are many the implementations of rdtime in different platform.
> In arch/arm/src/stm32/stm32_rtc_lowerhalf.c
>
> static int stm32_rdtime(FAR struct rtc_lowerhalf_s *lower,
> FAR struct rtc_time *rtctime)
> {
> #if defined(CONFIG_RTC_DATETIME)
> return **up_rtc_getdatetime**((FAR struct tm *)rtctime);
> `
> BR
Yes, all code inside drivers/timer/arch_*.c is to implement up_timer_ up_rtc_ on top of timer/oneshot/rtc driver interface. As I said before, if you want to use these files:
1.Remove up_rtc and up_timer_ up_alarm_ from your chipset
2.Implement the standard timer/oneshot/rtc driver interface
3.Let these file implement these function for you
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590079793
> Not a whole architecture, different chipset in the same architecture can select the different approach because each chipset has own Make.defs and can include/exclude up_delay.c as needed, but the same chipset must share the same decision.
Isn't that just an abritrary limitation? Why should implementation on any architecture have this option? Why limit it to just certain, pre-determined architectures?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589958626
This patch turns the architecture into spaghetti code and must not be permitted come into the system. It violates every concept of good modular design. Horrible idea.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 closed issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #353:
multiple definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590082027
> > Not a whole architecture, different chipset in the same architecture can select the different approach because each chipset has own Make.defs and can include/exclude up_delay.c as needed, but the same chipset must share the same decision.
>
> Isn't that just an abritrary limitation? Why should implementation on any architecture have this option? Why limit it to just certain, pre-determined architectures?
So you want to add an option to let user select?
1.Use up_rtc_*, up_timer_* and up_alarm_* in arch/ or
2.Use up_rtc_*, up_timer_* and up_alarm_* in drivers/timers/
There isn't difference outside the implementatin, the user get the same functionality with either selection.
Actually, up_timer_/up_rtc_/up_alarm_ API is duplicated with oneshot_operations_s/rtc_ops_s/timer_ops_s and make the chipset developer write many duplication without any benefit. The best solution is:
1.Remove up_timer_, up_alarm_, up_rtc from nuttx/arch.h
2.Modify the code under sched/ to call oneshot_operations_s/rtc_ops_s/timer_ops_s instead of up_timer_, up_alarm_, up_rtc.
3.Chipset developer just need implement oneshot_operations_s/rtc_ops_s/timer_ops_s for their hardware.
Basically, this approach move the code from drivers/timers/arch_* to sched/.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590084312
> So you want to add an option to let user select?
If they option is available to use, it should be safe and correct for them to select it and it should work correctly. If they are not supposed to select the option, then it should be disabled.
So either every architecture that does not support the drivers/timer/arch_* drivers should disable those options, or the architectures should permit the user to select them without anything bad happening.
That is simply a matter of making sure that the configuration options all work as advertised. It is make the configuration usable. It is not usable if you don't know if an option is available or not.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590074752
Not a whole architecture, different chipset in the same architecture can select the different approach because each chipset has own Make.defs and can include/exclude up_delay.c as needed, but the same chipset must share the same decision.
The code under drivers/timers/arch_*.c just want to simplify the chipset developer work, supporting both approach just make their work hard than before without any benefit.
So I don't think the last issue is a real limitation, the developer just need decide which method he want to use and forget another one totally before writing the code.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #353:
multiple definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589959094
Actually, it is drivers/timer/arch_timer.c and arch_rtc.c that ruined the modularity be using up_ prefix. But architectural thinking to begin with. But let's not make things worse. We keep all logic in "architectural silos" and we must prevent dependencies from spread across silos. That is forbidden by the INVIOLABLES.txt and the basic architectural principles of the OS. Modularity is the most important attribute of a good system and strictly enforcing modularity, not matter how painful, is the ONLY way to keep the architecture from degenerating into spaghetti code.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] klmchp commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
klmchp commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589938580
[fix_multi_definition_up_mdelay.zip](https://github.com/apache/incubator-nuttx/files/4239072/fix_multi_definition_up_mdelay.zip)
Hi ,
Upload a patch to fix this issue and please help to review it.
BR
Kevin Liu
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589957845
I don't like this patch. It adds CONFIG_ALARM_ARCH conditional logic to every architecture. That is wrong and I will not merge it. Please consider an alternative design.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589960471
So what kinds of things can we do now to prevent a dependency of all architectures on drivers/timer/arch_timer.c?
There is a precedent for something like CONFIG_ARCH_CUSTOM_MDELAY. Like this patch, every implementation of up_mdelay() would depend on "#ifdef CONFIG_ARCH_CUSTOM_MDELAY" and this would be defined in arch/Kconfig like:
config ARCH_CUSTOM_MDELAY
bool
default n
Then in drivers/timers/Kconfig, you could do
config TIMER_ARCH
bool "Timer Arch Implementation"
select ARCH_HAVE_TICKLESS
select ARCH_HAVE_TIMEKEEPING
select ARCH_CUSTOM_MDELAY
---help---
Implement timer arch API on top of timer driver interface.
This is logically the same. It is not elegant and it is not the quality of modularity that I would like to have. But at least it would prevent all architectures from being completely dependent on a particular external driver (which is not acceptable).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589959094
Actually, it is drivers/timer/arch_timer.c and arch_rtc.c that ruined the modularity be using up_ prefix. But architectural thinking to begin with.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589958104
The definitions of up_mdelay() should be removed from arch_timer.c. They don't belong there.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590072641
> 1.Remove up_delay.c from your chipset Make.defs directly
> 2.Enable CONFIG_ALARM_ARCH in your defconfig
That is not a good solution. The means that the configuration is not an option. Either an architecture must always have CONFIG_ALARM_ARCH and never build up_*delay.c; or it must never have CONFIG_ALARM_ARCH and always build up_*dealy.c.
That is not very flexible.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-589959532
Per the ENVIOLABLES.txt:
The Enemies
===========
o Doing things the easy way instead of the correct way.
o Reducing effort at the expense of Quality, Portability, or Consistency.
o It takes work to support the Inviolables. There are no shortcuts.
o Too much focus on solving the problem in hand, loss of the Big Picture.
o Insufficient understanding of the architectural principles.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590031621
But, arch code don't need reference any specific config for arch_timer.c/arch_rtc.c/arch_oneshot.c since this is design decision whether to use the implemetnetation inside these files:
1.Provide up_ timer/rtc API implmentation in arch/ or
2.Implement timer/oneshot/rtc driver and enable arch_timer.c/arch_rtc.c/arch_oneshot.c from defconfig
I don't think the developer want to implement both and let the user select from defconfig.
We don't need apply the change provided by @klmchp at all.
@klmchp if you want to use arch_timer.c, please:
1.Remove up_delay.c from your chipset Make.defs directly
2.Enable CONFIG_ALARM_ARCH in your defconfig
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590087053
Ok, maybe it is better to change CONFIG_ARCH_[TIMER|RTC|ALARM] to CONFIG_ARCH_HAVE_[TIMER|RTC|ALARM] without prompt string, than each chipset could select these option from Kconfig base on their implementation decision.
So it is impossible to make the wrong selection from defconfig.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590082027
> > Not a whole architecture, different chipset in the same architecture can select the different approach because each chipset has own Make.defs and can include/exclude up_delay.c as needed, but the same chipset must share the same decision.
>
> Isn't that just an abritrary limitation? Why should implementation on any architecture have this option? Why limit it to just certain, pre-determined architectures?
So you want to add an option to let user select?
1.Use up_rtc_*, up_timer_* and up_alarm_* in arch/ or
2.Use up_rtc_*, up_timer_* and up_alarm_* in drivers/timers/
There isn't difference outside the implementatin, the user get the same functionality with either selection.
Actually, up_timer_/up_rtc_/up_alarm_ API is duplicated with oneshot_operations_s/rtc_ops_s/timer_ops_s and make the chpset developer write many duplication without any benefit. The best solution is:
1.Remove up_timer_, up_alarm_, up_rtc from nuttx/arch.h
2.Modify the code under sched/ to call oneshot_operations_s/rtc_ops_s/timer_ops_s instead of up_timer_, up_alarm_, up_rtc.
3.Developer just need implement oneshot_operations_s/rtc_ops_s/timer_ops_s for their hardware
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590014127
That is a little better. However, I still think we need to avoid driver/ configuration settings under arch/ I would still prefer to see a separate CONFIG_ARCH_CUSTOM_MDELAY is I suggest above. This avoids any explicit coupling with drivers altogether.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] klmchp commented on issue #353: multiple
definition of `up_mdelay'
Posted by GitBox <gi...@apache.org>.
klmchp commented on issue #353: multiple definition of `up_mdelay'
URL: https://github.com/apache/incubator-nuttx/issues/353#issuecomment-590076414
@patacongo @xiaoxiang781216 , also find multi definition of up_rtc_getdatetime, up_rtc_settime and so on. I list the code from nuttx/drivers/timers/arch_rtc.c for reference.
`int up_rtc_getdatetime(FAR struct tm *tp)
{
if (g_rtc_lower != NULL)
{
struct rtc_time rtctime;
ret = g_rtc_lower->ops->rdtime(g_rtc_lower, &rtctime);
There are many the implementations of rdtime in different platform.
In arch/arm/src/stm32/stm32_rtc_lowerhalf.c
static int stm32_rdtime(FAR struct rtc_lowerhalf_s *lower,
FAR struct rtc_time *rtctime)
{
#if defined(CONFIG_RTC_DATETIME)
return **up_rtc_getdatetime**((FAR struct tm *)rtctime);
`
BR
----------------------------------------------------------------
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
With regards,
Apache Git Services