-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement core1 #112
base: main
Are you sure you want to change the base?
Implement core1 #112
Conversation
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 |
Thanks again! Is this ready for review or not yet? |
Yes, it's ready. |
Now I just need to find a few hours to go through it :) |
@urish Do you have time to check this PR? |
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. |
} | ||
|
||
stop() { | ||
this.stopped = true; | ||
this.core0.stopped = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stopped
flag for each core is necessary. Take pico-debug as an example, it use one core to debug another core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (!this.core0.stopped && !this.core0.waiting) { | ||
idle = false; | ||
this.isCore0Running = true; | ||
this.core0.executeInstruction(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();
}
}
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Edit:
|
If I understand correctly, the bootloader leaves core1 in waiting state, but Adding |
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. |
@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 |
@1570 Where is the code to bypass bootloader in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/peripherals/ppb.ts
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please call it coreIndex
(or similar) instead of _core
(same for writeUint32ViaCore)
@urish. To support for 2nd core with GDB, I just slightly traced code. Here are some issues to deal with.
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. |
Thanks for looking into this! Here are my thoughts:
So config a), and stop everything when we hit a breakpoint (or debugger sends a break command). |
If 2 cores always stopped simultaneously, do we need multi-thread to implement this feature? As I understanding, one thread is enough. |
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. |
#109
Test ok with pico-example code/multicore
Interpolator/Divider/Spinlock are not tested or implemented since I have no suitable code to verify.