Implement core1 #112

mingpepe · 2022-12-12T02:27:27Z

Test ok with pico-example code/multicore

Interpolator/Divider/Spinlock are not tested or implemented since I have no suitable code to verify.

urish · 2022-12-12T14:53:27Z

Thank you!

These is a pretty list big of changes, so it'll probably take me some time to go over everything.

Regarding Interpolator/Divider - as far as I remember, the only thing that we need is to have different one per core, so if two cores are using the divider / interpolator at the same time, they don't interfere with eachother:

So ideally, we'd want a set of Interpolator per core. It might make more sense to also move the divider logic into its own object, and then it'd be easier to create a dedicated copy of it for each core.

mingpepe · 2022-12-13T01:30:41Z

If interpolator and divider work properly for single core, I will work on it later to let each core has individual function.

But I do not have multi-core code about them to test, only single core(divider, interp).

urish · 2023-01-01T12:28:27Z

Thanks again! Is this ready for review or not yet?

mingpepe · 2023-01-01T16:46:50Z

Yes, it's ready.

urish · 2023-01-16T21:17:59Z

Now I just need to find a few hours to go through it :)

src/rp2040.ts

mingpepe · 2023-02-06T11:15:47Z

@urish Do you have time to check this PR?

urish · 2023-02-11T21:27:34Z

I added it to my todo list, I will make an effort to allocate some time this week or the next week.

Ideally, I'd also love to have the multicore test running as part of the CI. I've set up a GitHub action to run MicroPython tests two weeks ago, and hope to get some pico-sdk examples to run as part of the CI too.

c1570 · 2023-03-12T15:28:58Z

src/rp2040.ts

 }

 stop() {
- this.stopped = true;
+ this.core0.stopped = true;


As far as I understand, stopped is a bit of a kludge for controlling the emulation machinery (that's running via setTimeout()) from outside code; it's not a feature of the actual RP2040 hardware. Since the cores are not running as callbacks individually, there's probably no need to give each core a stopped attribute.

That said, I'd just remove the setTimeout() stuff from rp2040js altogether; it seems to be some leftover from Wokwi. It'd probably be better to just have a rp2040.step() function and leave everything else to the calling code. (but this might be outside the scope of this PR)

The stopped flag for each core is necessary. Take pico-debug as an example, it use one core to debug another core.

The pico-debug project uses the SYSCFG DBGFORCE register, basically rerouting the SWD functionality built into the (real, hardware) Cortex cores via USB. This is unrelated to stopped, and SWD functionality isn't implemented at all in rp2040js.

What I want to say is the 2 cores may be 1 running and 1 stopped. And you are right, pico-debug use SWD which is not related to our stopped flag. But you can check gdb server in this project which only stop core0 while debugging.

c1570 · 2023-03-12T15:34:20Z

src/rp2040.ts

+ if (!this.core0.stopped && !this.core0.waiting) {
+ idle = false;
+ this.isCore0Running = true;
+ this.core0.executeInstruction();


This eventually gets the cores out of sync. Imagine core0 executing a number of 5 cycle instructions and core 1 executing a number of 1 cycle instructions, let things run a while, then look at the cycles of each core (which will have diverged a lot).

You'll need something like https://github.com/c1570/rp2040js/blob/3a754941c169d7215d8d97112c01a9d1f9d4e2c9/demo/dual-mcu.ts#L102 here.

Yes, the two cores execution cycle may diverge if they run a while and the stopped or waiting flag raised. Currently I do not have good solution for it. But what you did may not be helpful(if I understand correctly).

If one core is stopped or waiting, these state will not changed until the next setTimeout call since there is only 1 thread in our execution environment. Then your code to call core.step actually do nothing.

Something like this is needed in cortex-m0-core.ts:

executeInstruction() { if (this.waiting) { this.cycles++; return; } ...

...and something like this in rp2040.ts (this is my local version with #117 already merged):

step() { let core0StartCycles = this.core0.cycles; this.isCore0Running = true; this.core0.executeInstruction(); this.isCore0Running = false; while(this.core1.cycles < this.core0.cycles) { this.core1.executeInstruction(); } for(let cycle = core0StartCycles; cycle < this.core0.cycles; cycle++) { this.pio[0].step(); this.pio[1].step(); } }

c1570 · 2023-03-12T15:36:36Z

src/rp2040.ts

@@ -48,7 +49,8 @@ export class RP2040 {
 readonly usbDPRAM = new Uint8Array(4 * KB);
 readonly usbDPRAMView = new DataView(this.usbDPRAM.buffer);

- readonly core = new CortexM0Core(this);
+ readonly core0 = new CortexM0Core(this);


Why not using an array/list for the cores? Seems that would make quite a few places of the code simpler, and RP4040 might be just around the corner anyways. ;)

At the beginning, I use array of cores, but found so may cases need to handle for each core(like notify another core interrupt or operations no SIO) which makes the benefit of array less and make the code not easy to read(compare to use 2 variable). It's just my opionion.

c1570 · 2023-03-12T18:34:21Z

Edit:

cmake -DPICO_COPY_TO_RAM=1 .. will break core1, even for programs using only core0. Not sure why yet.

c1570 · 2023-03-14T00:33:02Z

If I understand correctly, the bootloader leaves core1 in waiting state, but emulator-run.ts jumps into crt0 immediately, bypassing the bootloader. This makes core1 reach the code at https://github.com/raspberrypi/pico-sdk/blob/f396d05f8252d4670d4ea05c8b7ac938ef0cd381/src/rp2_common/pico_standard_link/crt0.S#L315 where things go wrong (possibly because core0 is still copying things to RAM at that point).

Adding mcu.core1.waiting = true; to emulator-run seems to fix the issue. :)

urish · 2023-03-14T09:32:47Z

Thanks for reviewing this @c1570! Unfortunately, I hadn't had a chance yet to review it myself.

I got the Pico SDK CI to work: https://github.com/wokwi/rp2040js/blob/master/.github/workflows/ci-pico-sdk.yml, which can be a good starting point for adding a multicore test to the CI.

c1570 · 2023-03-14T11:34:00Z

@urish How about just adding the needed example .hex files and use those? This would make tests easier/faster to run for users and allow testing things like -DPICO_COPY_TO_RAM more easily.

mingpepe · 2023-03-18T00:53:05Z

@1570 Where is the code to bypass bootloader in emulator-run.ts? I did not ever use -DPICO_COPY_TO_RAM before.

c1570 · 2023-03-18T01:03:49Z

@1570 Where is the code to bypass bootloader in emulator-run.ts? I did not ever use -DPICO_COPY_TO_RAM before.

rp2040js/demo/emulator-run.ts

Line 21 in 6344182

mcu.core.PC = 0x10000000;

jumps to 0x10000000 (the hex file contents) but the bootrom is at 0x0 basically (if I understand correctly).

mingpepe · 2023-03-18T01:15:08Z

@1570 Where is the code to bypass bootloader in emulator-run.ts? I did not ever use -DPICO_COPY_TO_RAM before.

rp2040js/demo/emulator-run.ts

Line 21 in 6344182

mcu.core.PC = 0x10000000;

jumps to 0x10000000 (the hex file contents) but the bootrom is at 0x0 basically (if I understand correctly).

Thanks.

urish

Thanks again for working on this!

Did a few tests, ran all the multicore examples, and they seem to work well. Also did a quick review of the code, left one minor comment.

Have you looked at adding support for debugging the 2nd core using GDB?

I believe the way to go is to expose each code as a different execution thread to GDB. This is essential so in the future we'll be able to debug issues involving both cores.

urish · 2023-03-19T22:46:22Z

src/peripherals/ppb.ts

@@ -53,9 +54,9 @@ export class RPPPB extends BasePeripheral implements Peripheral {
 systickReload = 0;
 systickTimer: IClockTimer | null = null;

- readUint32(offset: number) {
+ readUint32ViaCore(offset: number, _core: Core) {


please call it coreIndex (or similar) instead of _core (same for writeUint32ViaCore)

mingpepe · 2023-03-21T10:22:14Z

@urish. To support for 2nd core with GDB, I just slightly traced code. Here are some issues to deal with.

Which gdb server configuration do we need?
a) The official config use smp and hwthread which shows only 1 TCP port to gdb client
b) Expose 2 TCP ports to gdb clients => Two instance of GDBTCPServer with different port in our case.
Currently timers in RealTimeClock do not distinguish for which core owned. When one core is stopped, I'm not sure if it's timers should keep going or should be paused (like DMA).

For gdb server config a), when 1 break point hit, two cores are stopped (I do not know if it can config to let another core free run). I think in this way some gdb commands currently not implemented (but I do not know the details about gdb command yet). This may be complicated compare to config b) but let me debug my code with VSCode extension which is good for me. For config b), I'm not sure if VSCode extension can achieve connecting 2 gdb server.

urish · 2023-03-21T10:53:06Z

Thanks for looking into this! Here are my thoughts:

Which gdb server configuration do we need? single GDB port, multiple hw threads.
From my experience, pausing everything (both cores & all timers) makes life much easier when debugging. You want to have everything as predictable as possible, so if you set a breakpoint and step over, you want to get consistent results regardless of how fast you're stepping over.

So config a), and stop everything when we hit a breakpoint (or debugger sends a break command).

mingpepe · 2023-03-21T11:51:09Z

If 2 cores always stopped simultaneously, do we need multi-thread to implement this feature? As I understanding, one thread is enough.

urish · 2023-03-21T11:52:13Z

You want some way to show GDB the state of each core - so it would show you the call stack of each core, and let you step the core that you are interested it.

mingpepe added 16 commits November 28, 2022 11:51

Implement TBMAN

190cba8

Implement REF/SYS CTRL & SELECT for clock

de3601f

Define enum for cores

e7a222c

Add API for peripheral to read from core

dfbeaf0

Define onSEV event to wakeup another core

aca3a9c

Define onBreak event for GDB

1b660e3

Implement read/write via core for ppb

c64c141

Implement FIFO registers in SIO

d5a1074

Define 2 cores in rp2040

a31e8fc

Register onSEV event for each core

dc29d0c

Implement NMI_MASK for syscfg

efb7820

Modify debugger to debug only on core0

dcc32ab

Update unit test

26fb63f

Update test-utils

9b54d9d

Update demo code

b1b4560

Merge branch 'master' into core1

d12a4b5

mingpepe changed the title ~~Implement core1 #109~~ Implement core1 Dec 12, 2022

mingpepe added 5 commits December 13, 2022 07:37

Let each core has its divider

4a0cf99

Let each core has its interpolator

10ec3e6

Move fifo related logic to each core

fd563cd

Do no need to pass core info to each core

c9ab5ea

Add core info to warning message

6f2da85

c1570 reviewed Jan 21, 2023

View reviewed changes

src/rp2040.ts Outdated Show resolved Hide resolved

mingpepe added 2 commits January 30, 2023 11:27

Execute core0 and core1 interleaving

278bda7

Merge branch 'master' into core1

26f77aa

mingpepe added 3 commits February 6, 2023 02:44

Fix wrong merge

1290137

Run lint

9b55ad5

Fix error by lint

9bb7e7a

c1570 reviewed Mar 12, 2023

View reviewed changes

urish self-requested a review March 20, 2023 08:18

urish self-assigned this Mar 20, 2023

urish added the enhancement New feature or request label Mar 20, 2023

urish requested changes Mar 20, 2023

View reviewed changes

Rename variable

23bf9d3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement core1 #112

Implement core1 #112

mingpepe commented Dec 12, 2022 •

edited

urish commented Dec 12, 2022

mingpepe commented Dec 13, 2022 •

edited

urish commented Jan 1, 2023

mingpepe commented Jan 1, 2023

urish commented Jan 16, 2023

mingpepe commented Feb 6, 2023

urish commented Feb 11, 2023

c1570 Mar 12, 2023

mingpepe Mar 18, 2023

c1570 Mar 18, 2023

mingpepe Mar 18, 2023

c1570 Mar 12, 2023

mingpepe Mar 18, 2023 •

edited

c1570 Mar 18, 2023

c1570 Mar 12, 2023

mingpepe Mar 18, 2023

c1570 commented Mar 12, 2023 •

edited

c1570 commented Mar 14, 2023

urish commented Mar 14, 2023

c1570 commented Mar 14, 2023

mingpepe commented Mar 18, 2023 •

edited

c1570 commented Mar 18, 2023

mingpepe commented Mar 18, 2023

urish left a comment

urish Mar 19, 2023

mingpepe commented Mar 21, 2023

urish commented Mar 21, 2023

mingpepe commented Mar 21, 2023

urish commented Mar 21, 2023

Implement core1 #112

Are you sure you want to change the base?

Implement core1 #112

Conversation

mingpepe commented Dec 12, 2022 • edited

urish commented Dec 12, 2022

mingpepe commented Dec 13, 2022 • edited

urish commented Jan 1, 2023

mingpepe commented Jan 1, 2023

urish commented Jan 16, 2023

mingpepe commented Feb 6, 2023

urish commented Feb 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingpepe Mar 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c1570 commented Mar 12, 2023 • edited

c1570 commented Mar 14, 2023

urish commented Mar 14, 2023

c1570 commented Mar 14, 2023

mingpepe commented Mar 18, 2023 • edited

c1570 commented Mar 18, 2023

mingpepe commented Mar 18, 2023

urish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingpepe commented Mar 21, 2023

urish commented Mar 21, 2023

mingpepe commented Mar 21, 2023

urish commented Mar 21, 2023

mingpepe commented Dec 12, 2022 •

edited

mingpepe commented Dec 13, 2022 •

edited

mingpepe Mar 18, 2023 •

edited

c1570 commented Mar 12, 2023 •

edited

mingpepe commented Mar 18, 2023 •

edited