Rename attr

[haslinghuis]

Rename attr to attributes to make clear distinction between other uses of this name.

Follow up on #11261

[ledvinap]

Using attributes template in ROM will same few bytes of memory (no pointer) and save instruction or two on dereference... And with all the refactoring going on, it may now be worth it ...

[ledvinap]

Suggested change

	task_attributes_t task_attributes[TASK_COUNT] = {
	static const task_attributes_t task_attributes[TASK_COUNT] = {

[ledvinap]

Suggested change

	tasks[i].attributes = &task_attributes[i];
	tasks[i].attributes = task_attributes[i];

[ledvinap]

Suggested change

	if (taskQueueArray[ii] == NULL || taskQueueArray[ii]->attributes->staticPriority < task->attributes->staticPriority) {
	if (taskQueueArray[ii] == NULL || taskQueueArray[ii].attributes->staticPriority < task->attributes.staticPriority) {

[ledvinap]

Suggested change

	task_attributes_t *attributes;
	task_attributes_t attributes;

[haslinghuis]

@ledvinap I've tried to implement but got a lot of errors doing the suggestions.

I've ended here:
diff.txt

[haslinghuis]

Thanks to @daleckystepan for pointing out the following:

typedef struct {
    // Configuration
    const char * taskName;
    const char * subTaskName;
    bool (*checkFunc)(timeUs_t currentTimeUs, timeDelta_t currentDeltaTimeUs);
    void (*taskFunc)(timeUs_t currentTimeUs);
    timeDelta_t desiredPeriodUs;        // target period of execution
    int8_t staticPriority;        // dynamicPriority grows in steps of this size
} task_attributes_t;

staticPriority was defined as const and assignment violated that.

[haslinghuis]

@ledvinap Implemented your suggestion in second commit.

[ledvinap]

The changes seem safe.
Is it possible to post comparison of code size?

[haslinghuis]

This PR:

Linking STM32F411 
Memory region         Used Size  Region Size  %age Used
           FLASH:        8072 B        10 KB     78.83%
FLASH_CUSTOM_DEFAULTS:           8 B         6 KB      0.13%
    FLASH_CONFIG:          0 GB        16 KB      0.00%
          FLASH1:      487439 B       480 KB     99.17%
FLASH_CUSTOM_DEFAULTS_EXTENDED:          0 GB         0 GB
   SYSTEM_MEMORY:          0 GB        29 KB      0.00%
             RAM:      112136 B       128 KB     85.55%
       MEMORY_B1:          0 GB         0 GB
   text	   data	    bss	    dec	    hex	filename
 489919	   5600	 106136	 601655	  92e37	./obj/main/betaflight_STM32F411.elf

Master:

Linking STM32F411 
Memory region         Used Size  Region Size  %age Used
           FLASH:        8768 B        10 KB     85.62%
FLASH_CUSTOM_DEFAULTS:           8 B         6 KB      0.13%
    FLASH_CONFIG:          0 GB        16 KB      0.00%
          FLASH1:      486743 B       480 KB     99.03%
FLASH_CUSTOM_DEFAULTS_EXTENDED:          0 GB         0 GB
   SYSTEM_MEMORY:          0 GB        29 KB      0.00%
             RAM:      112248 B       128 KB     85.64%
       MEMORY_B1:          0 GB         0 GB
   text	   data	    bss	    dec	    hex	filename
 489223	   6296	 105552	 601071	  92bef	./obj/main/betaflight_STM32F411.elf

[ledvinap]

Hmm. seems that GCC insists on unrolling the struct assignment. This will probably reduce code size:

Suggested change

	tasks[i].attributes = task_attributes[i];
	memcpy_fn(tasks[i].attributes, task_attributes[i], sizeof(tasks[i].attributes);

[ledvinap]

Sorry, memcpy_fn(&tasks[i].attributes, &task_attributes[i], sizeof(tasks[i].attributes);

[haslinghuis]

Output:

Linking STM32F411 
Memory region         Used Size  Region Size  %age Used
           FLASH:        8072 B        10 KB     78.83%
FLASH_CUSTOM_DEFAULTS:           8 B         6 KB      0.13%
    FLASH_CONFIG:          0 GB        16 KB      0.00%
          FLASH1:      487431 B       480 KB     99.17%
FLASH_CUSTOM_DEFAULTS_EXTENDED:          0 GB         0 GB
   SYSTEM_MEMORY:          0 GB        29 KB      0.00%
             RAM:      112136 B       128 KB     85.55%
       MEMORY_B1:          0 GB         0 GB
   text	   data	    bss	    dec	    hex	filename
 489911	   5600	 106136	 601647	  92e2f	./obj/main/betaflight_STM32F411.elf

[ledvinap]

Hmm .. using memcpy has minimal effect:

.L1003:
	mov	lr, r5
	ldmia	lr!, {r0, r1, r2, r3}
	mov	ip, r6
	stmia	ip!, {r0, r1, r2, r3}
	ldm	lr, {r0, r1}
	adds	r5, r5, #24
	cmp	r8, r5
	stm	ip, {r0, r1}
	add	r6, r6, #80
	bne	.L1003

vs

.L1003:
	mov	r1, r6
	mov	r0, r5
	movs	r2, #24
	adds	r5, r5, #80
	bl	memcpy
	cmp	r7, r5
	add	r6, r6, #24
	bne	.L1003

[daleckystepan]

Master seems to be the best solution according to these tables.Let's stick with renaming only.

[ledvinap]

I just don't get where is size difference coming from. Compiler is saving 1 instruction on attributes access. But maybe 32bit instruction encoding has to be used with larger 'tasks', making code larger ...

[ledvinap]

Another possibility is that with smaller code, gcc decides to unroll / inline function somewhere ...

[haslinghuis]

No worries - at least we tried

[daleckystepan]

I was thinking about this. We don't necessary need to link initialized and uninitialized data together.

We can have

typedef struct {
    // Configuration
    const char * taskName;
    const char * subTaskName;
    bool (*checkFunc)(timeUs_t currentTimeUs, timeDelta_t currentDeltaTimeUs);
    void (*taskFunc)(timeUs_t currentTimeUs);
    timeDelta_t desiredPeriodUs;        // target period of execution
    const int8_t staticPriority;        // dynamicPriority grows in steps of this size
} task_id_t;

typedef struct {
    // Scheduling
    uint16_t dynamicPriority;           // measurement of how old task was last executed, used to avoid task starvation
    uint16_t taskAgeCycles;
    timeDelta_t taskLatestDeltaTimeUs;
    timeUs_t lastExecutedAtUs;          // last time of invocation
    timeUs_t lastSignaledAtUs;          // time of invocation event for event-driven tasks
    timeUs_t lastDesiredAt;             // time of last desired execution

    // Statistics
    float    movingAverageCycleTimeUs;
    timeUs_t anticipatedExecutionTime;  // Fixed point expectation of next execution time
    timeUs_t movingSumDeltaTime10thUs;  // moving sum over 64 samples
    timeUs_t maxExecutionTimeUs;
    timeUs_t totalExecutionTimeUs;      // total time consumed by task since boot
    timeUs_t lastStatsAtUs;             // time of last stats gathering for rate calculation
#if defined(USE_LATE_TASK_STATISTICS)
    uint32_t runCount;
    uint32_t lateCount;
    timeUs_t execTime;
#endif
} task_t;

// Task info in .bss (unitialised data)
task_t tasks[TASK_COUNT];

And then we can use (Maybe some macro or inline function)

const task_id_t *getTaskAttributes(const task_t *task) {
    return &task_ids[tasks-task];
}

or just

task_ids[id]

where applicable. We can save dereference, memcpy, ...

[ledvinap]

@daleckystepan : IMO not worth it .. The code will be too confusing and it is only minor optimization

[haslinghuis]

So I guess we purge commit 2 here and leave it with renaming.

[haslinghuis]

Removed second commit:

Linking STM32F411 
Memory region         Used Size  Region Size  %age Used
           FLASH:        8768 B        10 KB     85.62%
FLASH_CUSTOM_DEFAULTS:           8 B         6 KB      0.13%
    FLASH_CONFIG:          0 GB        16 KB      0.00%
          FLASH1:      486743 B       480 KB     99.03%
FLASH_CUSTOM_DEFAULTS_EXTENDED:          0 GB         0 GB
   SYSTEM_MEMORY:          0 GB        29 KB      0.00%
             RAM:      112248 B       128 KB     85.64%
       MEMORY_B1:          0 GB         0 GB
   text	   data	    bss	    dec	    hex	filename
 489223	   6296	 105552	 601071	  92bef	./obj/main/betaflight_STM32F411.elf

[haslinghuis]

@KarateBrot fixed. As it had multiple properties I thought it made sense to make all plural.

[asizon]

This is a refactor pr @daleckystepan, why include in 4.3 at this point?

[daleckystepan]

@asizon Just name change and labeled as 4.3