Freertos working commands by Known4225 · Pull Request #409 · Severson-Group/AMDC-Firmware

Known4225 · 2024-07-17T21:26:28Z

This temporary branch implements the following features in freertos:

uart communication
command handler
blink app

…red folder

Known4225 · 2024-07-23T18:58:25Z

@npetersen2 ready for review

@codecubepi please review code location under the shared/ folder

npetersen2

Thanks @Known4225 , I think this is great! I reviewed each file that changed and left some comments as appropriate.

Overall, there are several small things I'd like you to do, but in general, this is looking pretty close to merging from my perspective.

Let's see what @codecubepi has to say in his review

npetersen2 · 2024-07-29T22:34:38Z

sdk/FreeRTOS-Kernel

It appears the .gitmodules has to been changed, so why this change for the submodule here?

We should be trying to keep to named version releases from FreeRTOS---which version number does this change correspond to?

npetersen2 · 2024-07-29T22:36:26Z

sdk/shared/sys/serial.h

-
 #define SERIAL_UPDATES_PER_SEC (10000)
-#define SERIAL_INTERVAL_USEC   (USEC_IN_SEC / SERIAL_UPDATES_PER_SEC)
+#define SERIAL_INTERVAL_TICKS  (pdMS_TO_TICKS(1000.0 / SERIAL_UPDATES_PER_SEC))


Include header for pdMS_TO_TICKS macro

npetersen2 · 2024-07-29T22:38:55Z

sdk/shared/sys/serial.c

+	// Create serial task
+	xTaskCreate(serial_main, (const char *) "uartSerial", configMINIMAL_STACK_SIZE,
+				NULL, tskIDLE_PRIORITY, &xSerialTaskHandle);


Let's make a new header file that has all the task priorities in one place so we can reason about how the system operates easily.

Let's call it sys/task_priority.h and define priority there, then include and reference these defines in this file from each place that tasks are created.

Note: this comment applies to all the FreeRTOS tasks you have added, not just this one! There should be numerous priorities defines in this new file.

npetersen2 · 2024-07-29T22:40:03Z

sdk/shared/sys/intr.h

+#include <stdint.h>
+#include <stdio.h>
+
+// test


Remove this line

npetersen2 · 2024-07-29T22:42:13Z

sdk/shared/sys/intr.h

+// Interrupt Controller Instance
+//   Defined here to be accessable in sys/icc.c
+XScuGic InterruptController;


Scary scary! We should not be defining objects in header files.

Who "owns" this object? Where does this object live?

This code probably already was there before this PR... but I see you moved static.

Either way, we need to move this to a single .c file, then expose a extern declaration in the header

This is probably my bad...

npetersen2 · 2024-07-29T23:00:49Z

sdk/freertos_app_cpu0/src/usr/user_config.h

Interesting, why this empty file?

I suppose future modules you will port over will need this?

It looks like this is replacing the old user_config.h file? Perhaps this should be a shared/ resource going forward?

npetersen2 · 2024-07-29T23:03:20Z

sdk/freertos_app_cpu0/src/usr/game/task_game.h

+// Microseconds interval between when task is called
+//
+// This is what scheduler actually uses to run task,
+// but is generated via define above
+#define TASK_GAME_INTERVAL_USEC (USEC_IN_SEC / TASK_GAME_UPDATES_PER_SEC)


I noticed your game task has a different way of defining its interval_usec stuff versus the other system tasks.

Eventually, we should be consistent on the "recommended" way of creating tasks.

If we want your game to be kept in the codebase for the full v2 release, we'll need to update this. Otherwise, if the game will get removed, it does not matter

Yeah, do we need the "game" in the codebase? This could become confusing clutter in the future if it's not a required resource.

npetersen2 · 2024-07-29T23:04:13Z

sdk/freertos_app_cpu0/src/usr/game/cmd/cmd_game.c

Interesting commands here :)

npetersen2 · 2024-07-29T23:05:15Z

sdk/freertos_app_cpu0/src/main.c

-    } else {
-        message_status = 2;
-        xil_printf("CPU0 - FreeRTOS Hello World Example FAILED\r\n");
+    	xil_printf("IF YOU'RE READING THIS THEN A TERRIBLE ERROR HAS OCCURRED!\n");


Ha nice. :)

I am happy to keep this, but let's give the user a little more help here. Revise to say something about memory

I need the deleted code to be kept as well, see above comment on deleted code. I think you can probably add a task for your own testing without deleting my tasks.

npetersen2 · 2024-07-29T23:07:22Z

sdk/freertos_app_cpu0/src/main.c

-#if 1
-    // CPU0 ONLY:
-    // This code is required to start CPU1 from CPU0 during boot.
-    //
-    // This only applies when booting from flash via the FSBL.
-    // During development with JTAG loading, these low-level
-    // calls in this #if block are not needed! However, we'll
-    // keep them here since it doesn't affect performance...
-
-    // Write starting base address for CPU1 PC.
-    // It will look for this address upon waking up
-    static const uintptr_t CPU1_START_ADDR = 0xFFFFFFF0;
-    static const uint32_t CPU1_BASE_ADDR = 0x20080000;
-    Xil_Out32(CPU1_START_ADDR, CPU1_BASE_ADDR);
-
-    // Waits until write has finished
-    // DMB = Data Memory Barrier
-    dmb();
-
-    // Wake up CPU1 by sending the SEV command
-    // SEV = Set Event, which causes CPU1 to wake up and jump to CPU1_BASE_ADDR
-    __asm__("sev");
-#endif
-


Let's keep this code

Yes please keep... if this is deleted, we'll get nasty merge conflicts when I merge the dual-core stuff.

codecubepi

@Known4225

Good work so far. However it looks like many of your files have either changed minimally or not at all, but are marked as changed, probably due to line endings. If you could revert these EOL changes it will minimize the number of files changed, make review much easier, and Git will not have to store/track meaningless changes.

codecubepi · 2024-07-30T20:13:57Z

sdk/freertos_app_cpu0/src/usr/user_config.h

It looks like this is replacing the old user_config.h file? Perhaps this should be a shared/ resource going forward?

codecubepi · 2024-07-30T20:14:53Z

sdk/freertos_app_cpu0/src/usr/user_apps.h

@npetersen2 Perhaps this should be a shared/ resource going forward?

codecubepi · 2024-07-30T20:15:32Z

sdk/freertos_app_cpu0/src/usr/user_apps.c

Perhaps this should be a shared/ resource?

codecubepi · 2024-07-30T20:16:50Z

sdk/freertos_app_cpu0/src/usr/game/task_game.h

+// Microseconds interval between when task is called
+//
+// This is what scheduler actually uses to run task,
+// but is generated via define above
+#define TASK_GAME_INTERVAL_USEC (USEC_IN_SEC / TASK_GAME_UPDATES_PER_SEC)


Yeah, do we need the "game" in the codebase? This could become confusing clutter in the future if it's not a required resource.

codecubepi · 2024-07-30T20:17:39Z

sdk/freertos_app_cpu0/.project

Hmmm... we really need to determine a way to ditch these absolute paths :(

Probably fine for now though.

codecubepi · 2024-07-30T20:49:46Z

sdk/freertos_app_cpu1/src/FreeRTOSConfig.h

Again, what was changed in this file? Line endings?

@Known4225 please be conscious when you commit files of exactly what is changing. I recommend using the Diff View under 'Source Control' in VS Code...

If the only thing changing is line endings, please don't commit the change :)

codecubepi · 2024-07-30T20:50:29Z

sdk/freertos_app_cpu1/src/Xilinx.spec

See other comments on line endings

codecubepi · 2024-07-30T20:50:52Z

sdk/shared/FreeRTOS_tick_config.c

Line endings?

codecubepi · 2024-07-30T20:51:07Z

sdk/shared/README.md

Line endings

codecubepi · 2024-07-30T20:51:58Z

sdk/shared/sys/intr.h

+// Interrupt Controller Instance
+//   Defined here to be accessable in sys/icc.c
+XScuGic InterruptController;


This is probably my bad...

…obber each other and the lights look funny, but this is expected behaviour

Known4225 · 2024-08-02T18:34:44Z

Changes:

created task_priority.h file with task priorities for serial, cmds, and the blink and vsi tasks.
deleted game app
added user_config content
removed all line endings and permission issues (git can't seem to figure out that task_blink.c, task_vsi.c, task_vsi.h, and commands.c have been moved AND changed. So it lists them as deleted and re-added with different contents)
slight changes to FreeRTOSConfig.h (enabling run time statistics and providing a timer to use)
Added timer_stats for run time statistics timer
fixed intr.h header definition (now an extern declaration and definition in the source file)
included "projdefs.h" in commands.h to provide path to pdMS_TO_TICKS macro

temp branch to reduce file changes

e834c5d

Known4225 requested review from elsevers and npetersen2 as code owners July 17, 2024 21:26

Known4225 changed the base branch from v1.2.x to v2-staging July 17, 2024 21:28

Known4225 added 2 commits July 20, 2024 13:38

checking if git is smart enough to figure this out

197dc3f

some files removed from appcpu1 so they show up as renames in the sha…

750a0d4

…red folder

codecubepi self-requested a review July 29, 2024 21:03

codecubepi assigned Known4225 Jul 29, 2024

codecubepi added the v2 For FreeRTOS codebase label Jul 29, 2024

npetersen2 suggested changes Jul 29, 2024

View reviewed changes

codecubepi suggested changes Jul 30, 2024

View reviewed changes

Known4225 added 4 commits August 2, 2024 12:05

removed game app

04787c1

readded queue and blinky tasks to main.c

ac4b7d3

converted LF to CRLF in shared/sys files

fba5b62

okay that didn't work

759f8c5

Known4225 force-pushed the feature/freertos-singlecore-temp branch from 71beffc to 759f8c5 Compare August 2, 2024 17:34

Known4225 added 10 commits August 2, 2024 12:36

linux permissions

8fa23c1

line endings on cpu0

3a1362b

line endings on cpu1 and shared

d44789d

permissions on cpu1

5829e3e

linux permissions on shared

88659f1

permissions on blink

b9fe118

platform config

9f6ec24

include projdefs in commands.h

4fcb91f

removed vscode folder

d573a30

runtime statistics implemented, ran the code on AMDC and the tasks cl…

6a946b8

…obber each other and the lights look funny, but this is expected behaviour

Known4225 requested review from codecubepi and npetersen2 August 2, 2024 18:34

Conversation

Known4225 commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Known4225 commented Jul 23, 2024 • edited by codecubepi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

npetersen2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecubepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Known4225 commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Known4225 commented Jul 17, 2024 •

edited

Loading

Known4225 commented Jul 23, 2024 •

edited by codecubepi

Loading

Known4225 commented Aug 2, 2024 •

edited

Loading