Re: [PATCH 13/18] config: Add _Thread_Idle_body

[Chris Johns]

On 16/2/20 6:12 pm, Sebastian Huber wrote:
> On 16/02/2020 00:36, Chris Johns wrote:
> 
>> On 2020-02-15 03:02, Sebastian Huber wrote:
>>> Move the idle thread body configuration constant out of the
>>> configuration table.
>>>
>>> Provide a default definition of the idle thread body constant.
>>
>> May be I am missing something simple here. How would I provide an custom IDLE
>> task? Is it by providing something like ...
>>
>> #include 
>>
>> const Thread_Idle_body _Thread_Idle_body = My_Idle_body; 
> 
> No, definitely not. An application designer should use the documented API:
> 
> https://docs.rtems.org/branches/master/c-user/configuring_a_system.html#configure-idle-task-body
> 
> The goal of this and follow up change sets is to split this 3k LOC file
> (confdefs.h) in easier to review pieces.
> 
> The file contains also comments which should be in the Classic API Guide, e.g.
> how to configure the filesystems:
> 
> https://docs.rtems.org/branches/master/c-user/configuring_a_system.html#file-system-configuration-parameters
> 
> This is just an enumeration of the options. The user has no idea how things 
> are
> related after reading this section.
> 

Thank you for the explanation. I was reviewing the patch and not the patch and
the results of it being applied.

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 13/18] config: Add _Thread_Idle_body

[Gedare Bloom]

On Sun, Feb 16, 2020 at 12:30 AM Sebastian Huber <
sebastian.hu...@embedded-brains.de> wrote:

>
> On 16/02/2020 05:31, Gedare Bloom wrote:
>
> On Sat, Feb 15, 2020 at 4:36 PM Chris Johns  wrote:
>
>> On 2020-02-15 03:02, Sebastian Huber wrote:
>> > Move the idle thread body configuration constant out of the
>> > configuration table.
>> >
>> > Provide a default definition of the idle thread body constant.
>>
>> May be I am missing something simple here. How would I provide an custom
>> IDLE task? Is it by providing something like ...
>>
>> #include 
>>
>> const Thread_Idle_body _Thread_Idle_body = My_Idle_body;
>>
>>
> I think you do it by a configure macro that confdefs picks up by this line
> added:
> +const Thread_Idle_body _Thread_Idle_body = CONFIGURE_IDLE_TASK_BODY;
>
> It needs some doco ;)
>
> Sorry, my problem is that I am so familiar with this stuff that I don't
> need documentation. What do you want to have documented and were should it
> be placed?
>
> When I see a variable/function definition and want to see the Doxygen I
> just hit an editor key and get it. I don't want to duplicate documentation
> at the variable/function definition level.
>
 Well,
https://docs.rtems.org/branches/master/c-user/configuring_a_system.html#configure-idle-task-body
hasn't
seemed to change, so maybe that is just the point to make in the commit.
When we touch things that manipulate configuration, it is important to
understand whether or not it impacts the API. Sometimes that is hard for us
to tell in patch review just by looking quickly.

In the broader sense, the method used to initialize the idle task should be
explained at
https://docs.rtems.org/branches/master/c-user/initialization.html#the-idle-task
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 13/18] config: Add _Thread_Idle_body

[Sebastian Huber]

On 16/02/2020 05:31, Gedare Bloom wrote:
On Sat, Feb 15, 2020 at 4:36 PM Chris Johns > wrote:


On 2020-02-15 03:02, Sebastian Huber wrote:
> Move the idle thread body configuration constant out of the
> configuration table.
>
> Provide a default definition of the idle thread body constant.

May be I am missing something simple here. How would I provide an
custom
IDLE task? Is it by providing something like ...

#include 

const Thread_Idle_body _Thread_Idle_body = My_Idle_body;


I think you do it by a configure macro that confdefs picks up by this 
line added:

+const Thread_Idle_body _Thread_Idle_body = CONFIGURE_IDLE_TASK_BODY;

It needs some doco ;)


Sorry, my problem is that I am so familiar with this stuff that I don't 
need documentation. What do you want to have documented and were should 
it be placed?


When I see a variable/function definition and want to see the Doxygen I 
just hit an editor key and get it. I don't want to duplicate 
documentation at the variable/function definition level.


___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 13/18] config: Add _Thread_Idle_body

[Sebastian Huber]

On 16/02/2020 00:36, Chris Johns wrote:


On 2020-02-15 03:02, Sebastian Huber wrote:

Move the idle thread body configuration constant out of the
configuration table.

Provide a default definition of the idle thread body constant.


May be I am missing something simple here. How would I provide an 
custom IDLE task? Is it by providing something like ...


#include 

const Thread_Idle_body _Thread_Idle_body = My_Idle_body; 


No, definitely not. An application designer should use the documented API:

https://docs.rtems.org/branches/master/c-user/configuring_a_system.html#configure-idle-task-body

The goal of this and follow up change sets is to split this 3k LOC file 
(confdefs.h) in easier to review pieces.


The file contains also comments which should be in the Classic API 
Guide, e.g. how to configure the filesystems:


https://docs.rtems.org/branches/master/c-user/configuring_a_system.html#file-system-configuration-parameters

This is just an enumeration of the options. The user has no idea how 
things are related after reading this section.


___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 13/18] config: Add _Thread_Idle_body

[Gedare Bloom]

On Sat, Feb 15, 2020 at 4:36 PM Chris Johns  wrote:

> On 2020-02-15 03:02, Sebastian Huber wrote:
> > Move the idle thread body configuration constant out of the
> > configuration table.
> >
> > Provide a default definition of the idle thread body constant.
>
> May be I am missing something simple here. How would I provide an custom
> IDLE task? Is it by providing something like ...
>
> #include 
>
> const Thread_Idle_body _Thread_Idle_body = My_Idle_body;
>
>
I think you do it by a configure macro that confdefs picks up by this line
added:
+const Thread_Idle_body _Thread_Idle_body = CONFIGURE_IDLE_TASK_BODY;

It needs some doco ;)



> Chris
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [PATCH 13/18] config: Add _Thread_Idle_body

[Chris Johns]

On 2020-02-15 03:02, Sebastian Huber wrote:

Move the idle thread body configuration constant out of the
configuration table.

Provide a default definition of the idle thread body constant.


May be I am missing something simple here. How would I provide an custom 
IDLE task? Is it by providing something like ...


#include 

const Thread_Idle_body _Thread_Idle_body = My_Idle_body;

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

[PATCH 13/18] config: Add _Thread_Idle_body

[Sebastian Huber]

Move the idle thread body configuration constant out of the
configuration table.

Provide a default definition of the idle thread body constant.

Update #3875.
---
 cpukit/Makefile.am  |  1 +
 cpukit/include/rtems/confdefs.h |  7 +++---
 cpukit/include/rtems/config.h   |  8 +--
 cpukit/include/rtems/score/threadidledata.h | 13 +++
 cpukit/score/src/threadcreateidle.c |  3 +--
 cpukit/score/src/threadidledefault.c| 34 +
 6 files changed, 54 insertions(+), 12 deletions(-)
 create mode 100644 cpukit/score/src/threadidledefault.c

diff --git a/cpukit/Makefile.am b/cpukit/Makefile.am
index 13ddeeab51..33cb9e843e 100644
--- a/cpukit/Makefile.am
+++ b/cpukit/Makefile.am
@@ -956,6 +956,7 @@ librtemscpu_a_SOURCES += score/src/threaddispatch.c
 librtemscpu_a_SOURCES += score/src/threadget.c
 librtemscpu_a_SOURCES += score/src/threadhandler.c
 librtemscpu_a_SOURCES += score/src/threadinitialize.c
+librtemscpu_a_SOURCES += score/src/threadidledefault.c
 librtemscpu_a_SOURCES += score/src/threadidlestacksizedefault.c
 librtemscpu_a_SOURCES += score/src/threadloadenv.c
 librtemscpu_a_SOURCES += score/src/threadrestart.c
diff --git a/cpukit/include/rtems/confdefs.h b/cpukit/include/rtems/confdefs.h
index 9e166b39f3..1dfc06e158 100644
--- a/cpukit/include/rtems/confdefs.h
+++ b/cpukit/include/rtems/confdefs.h
@@ -1114,10 +1114,12 @@ extern "C" {
 #ifndef CONFIGURE_IDLE_TASK_BODY
   #if defined(BSP_IDLE_TASK_BODY)
 #define CONFIGURE_IDLE_TASK_BODY BSP_IDLE_TASK_BODY
-  #else
-#define CONFIGURE_IDLE_TASK_BODY _CPU_Thread_Idle_body
   #endif
 #endif
+
+#if defined(CONFIGURE_INIT) && defined(CONFIGURE_IDLE_TASK_BODY)
+const Thread_Idle_body _Thread_Idle_body = CONFIGURE_IDLE_TASK_BODY;
+#endif
 /**@}*/ /* end of IDLE thread configuration */
 
 /**
@@ -2659,7 +2661,6 @@ struct _reent *__getreent(void)
*/
   const rtems_configuration_table Configuration = {
 CONFIGURE_EXECUTIVE_RAM_SIZE, /* required RTEMS workspace */
-CONFIGURE_IDLE_TASK_BODY, /* user's IDLE task */
 #ifdef CONFIGURE_UNIFIED_WORK_AREAS   /* true for unified work areas */
   true,
 #else
diff --git a/cpukit/include/rtems/config.h b/cpukit/include/rtems/config.h
index 62804be41c..017b4ed476 100644
--- a/cpukit/include/rtems/config.h
+++ b/cpukit/include/rtems/config.h
@@ -87,12 +87,6 @@ typedef struct {
*/
   uintptr_t  work_space_size;
 
-  /** 
-   * This element points to the BSP's optional idle task which may override
-   * the default one provided with RTEMS.
-   */
-  void*(*idle_task)( uintptr_t );
-
   /**
* @brief Specifies if a unified work area is used or not.
*
@@ -155,7 +149,7 @@ uint32_t rtems_configuration_get_maximum_extensions( void );
 (_Watchdog_Ticks_per_timeslice)
 
 #define rtems_configuration_get_idle_task() \
-(Configuration.idle_task)
+(_Thread_Idle_entry)
 
 #define rtems_configuration_get_idle_task_stack_size() \
 (_Thread_Idle_stack_size)
diff --git a/cpukit/include/rtems/score/threadidledata.h 
b/cpukit/include/rtems/score/threadidledata.h
index aa39466f7f..c9dbbe2a0e 100644
--- a/cpukit/include/rtems/score/threadidledata.h
+++ b/cpukit/include/rtems/score/threadidledata.h
@@ -61,6 +61,19 @@ extern "C" {
  */
 extern const size_t _Thread_Idle_stack_size;
 
+/**
+ * @brief The idle thread body type.
+ */
+typedef void *( *Thread_Idle_body )( uintptr_t );
+
+/**
+ * @brief The idle thread body.
+ *
+ * This constant is defined by the application configuration via
+ * .
+ */
+extern const Thread_Idle_body _Thread_Idle_body;
+
 /** @} */
 
 #ifdef __cplusplus
diff --git a/cpukit/score/src/threadcreateidle.c 
b/cpukit/score/src/threadcreateidle.c
index 66ed92702e..52c3ad4534 100644
--- a/cpukit/score/src/threadcreateidle.c
+++ b/cpukit/score/src/threadcreateidle.c
@@ -25,7 +25,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include 
 
@@ -79,7 +78,7 @@ static void _Thread_Create_idle_for_CPU( Per_CPU_Control *cpu 
)
 
   idle->is_idle = true;
   idle->Start.Entry.adaptor = _Thread_Entry_adaptor_idle;
-  idle->Start.Entry.Kinds.Idle.entry = rtems_configuration_get_idle_task();
+  idle->Start.Entry.Kinds.Idle.entry = _Thread_Idle_body;
 
   _Thread_Load_environment( idle );
 
diff --git a/cpukit/score/src/threadidledefault.c 
b/cpukit/score/src/threadidledefault.c
new file mode 100644
index 00..21559656e0
--- /dev/null
+++ b/cpukit/score/src/threadidledefault.c
@@ -0,0 +1,34 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Copyright (C) 2020 embedded brains GmbH
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistr