multiple definition of `up_mdelay' about nuttx

[klmchp]

fix_multi_definition_up_mdelay.zip
Hi ,
Upload a patch to fix this issue and please help to review it.
BR
Kevin Liu

[patacongo]

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.

[patacongo]

The definitions of up_mdelay() should be removed from arch_timer.c. They don't belong there.

[patacongo]

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.

[patacongo]

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.

[patacongo]

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.

[patacongo]

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).

[xiaoxiang781216]

@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.

[patacongo]

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.

[xiaoxiang781216]

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

[patacongo]

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.

[xiaoxiang781216]

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.

[klmchp]

@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

[patacongo]

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?

[xiaoxiang781216]

@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

[xiaoxiang781216]

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/.

[patacongo]

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.

[xiaoxiang781216]

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.