Modify scheduler to be configurable between ISR sources per new timing manager

[npetersen2]

The scheduler ISR which drives the full v1 code now has two possible sources:

1. Existing method: time-based ISR which is arbitrarily configurable to any sample rate (defaults to 10 kHz), and is NOT synchronized to the FPGA sensor I/O.
2. NEW method: timing manager ISR which is triggered when all the enabled sensors have completed their sensing, and IS synchronized to the FPGA sensor I/O.

Let's add some logic in the sys/scheduler module which enables the user to pick their desired source for the ISR. This should use the usr/user_config.h system (i.e., not changeable at run-time).

The new default behavior when v1.3.0 is released should be to use the timing manager ISR as the source, and configure the ISR rate and PWM frequency to match the current default, i.e., 100 kHz PWM and 10 kHz ISR. The ISR should only fire on the bottom of the PWM triangle carrier.

This will be only a C code change.

TODO:

• [x] Read through #170 and see what applies here; get a good idea of how the scheduler works and how it will work in conjunction with the new timing manager
• [x] Implement the points in #314 that have not been yet implemented, as that is more or less the same issue (except it instead looks like it sets up the new ISR as the only one)
  • [x] Set the default user ratio to 10 as per # 314 states - the default PWM carrier freq is 100 kHz, but the default control rate is 10 kHz. As per above, I'll also change the default to be trigger on PWM low, whereas right now its just both.
  • [x] "change the initialization code which used to call timer_init() from scheduler_init()--- now it should call the new driver to set the ratio to the default 10 value" - this should be the new method, where the timing manager ISR is used. The old method is the time-based ISR which is arbitrarily configurable to any sample rate (defaults to 10 kHz), and is NOT synchronized to the FPGA sensor I/O, which I believe currently should still be calling timer_init(), which configures the scheduler_timer_isr.
  • [x] Make sure the sys/scheduler internal state stays intact---it keeps track of elapsed usec in order to know when to trigger the registered tasks. The new ISR should update the scheduler state so that every still works as desired.
• [x] Add a macro to user_config.h to choose the ISR source upon system startup
• [x] Test:
  • [x] The user should be able to change the ratio and see their control code run at a different subrate of the PWM carrier.
  • [x] "Validate this by running the baseline blink user application and confirm that it still runs at its desired rate (i.e., 5 Hz). Use the task timing stats or the drv/cpu_timer to check. Change the PWM switching frequency and ensure nothing changes with the user task rate."
  • [x] This comment

[annikaolson]

I originally had a question but I think as I've been typing I've answered my own question so I'm still just going to leave my comments here as notes on how this all works/my thought process in the implementation.

I'll see if the implementation I'm trying out actually works on Friday when I get into the lab.

As I've been figuring out how to implement this best, I'd like to point to these two steps in #314:

5. Remove the Xilinx Timer IP core and the drv/timer module
6. Change the initialization code which used to call timer_init() from scheduler_init()--- now it should call the new driver to set the ratio to the default 10 value.

I know fpga_timer.c was removed as that was what was associated with the old IP core, but should timer.c still be removed in this instance? [no]. Since we haven't touched the scheduler yet, its still there, and scheduler_init() calls timer_init(), which initializes the scheduler timer ISR and initializes everything in the timer. But it seems like we're still keeping this all as an option and just making it not the default. So should the scheduler_timer_isr() and the initialization in timer_init(...) still occur, just if they select that as the source? [yes. we would only be removing the timer/old scheduler method if this weren't v1].

I guess my initial understanding was that the old method uses that private SysTick timer instance (timer.c) which is linked to scheduler_timer_isr and updates the elapsed_usecand scheduler state, but the new method instead does not use timer.c and the scheduler_timer_isr, and instead uses the timing manager timing to synchronize it with the sensors, and should update the elapsed time and scheduler state in the timing manager's isr (isr_0 (which should be renamed)) in timing_manager.c, so the different ISRs are in different files.

I also don't know if it should instead all be in scheduler_timer_isr and the #if just determines whether scheduler_timer_isr is initalialized/connected to the timer interrupt source or the timing manager interrupt source (e.g. call either timer_init(scheduler_timer_isr) or timing_manager_init(scheduler_timer_isr)). This actually might be preferable, I would just need to add another #if USER_CONFIG_ISR_SOURCE into the scheduler_timer_isr to also also call the two functions on the timing manager's ISR.

I'll test and see what works better.

Since the timing manager is currently the only way to actually trigger any of the sensors (they can't start or collect any data without sending a trigger signal) the sensors should still be triggered. So I have the default user ratio at 10 (100kHz carrier freq : 10kHz control rate) and triggering on pwm low.

Then I guess if they're using the regular SysTick timer for the scheduler interrupt, this should be the default, and the ISR will still be called based on an arbitrarily configurable sample rate (defaults to 10 kHz). But the sensors can still be used with the timing manger.

If they're using the timing manager, then that is what triggers the scheduler ISR (currently, an isr_0 that sets the stats for the sensors is called) and takes care of the scheduler statuses as well. In this case, timer.c isn't really used and shouldn't be initialized/initializing the scheduler ISR.

[annikaolson]

So the only thing I'll have to see is if timer.c is needed at all if not used for the scheduler interrupt. I don't think it is, so I think something like this might work:

So the timing manager stuff is still initialized fine and called elsewhere, but just the interrupt is set up if chosen for the ISR source.

I think the elapsed_usec would also need to change then based on which source is being used if the timing manager isn't using the SysTick timer anymore.

https://github.com/Severson-Group/AMDC-Firmware/blob/153dd3f76416b4752a0acfcac4cb20288b142683/sdk/app_cpu1/common/sys/scheduler.c#L56-L57

[annikaolson]

When the SysTick timer is used for the scheduler ISR source, the timing manager can be used to enable sensors and such, but it can't actually report any time since that's only updated when the ISR is called...and now the ISR attached is the scheduler, which isn't configured in this mode.

• Solution:
  • [x] Can pass in the old ISR (isr_0 now renamed to timing_manager_isr) for the tm_interrupt_system_init so that ISR is still getting called and updating the stats, but also call timer_init with the scheduler_timer_isr. When tested, all of the timing stats are updated like normal as timing_manager_isr is triggered by the timing manager interrupt, and scheduler_timer_isr is triggered by the SysTick timer (previous functionality)

[npetersen2]

Some notes from out meeting today:

• TM mode concept only exists in the FPGA which determines when the ISR get triggered.
• The C code doesn't have any notion of what mode it is in

// Exactly when this ISR is called depends on the TM mode:
// "OLD" mode is called BEFORE the sensors finish ACQ
// "NEW" mode is called AFTER the sensors finish ACQ
TM_ISR() {
   tm_sensors_stats();
   scheduler_tick();
}

// With these changes, now scheduler does this:
scheduler_tick() {
  // First, keep track of elapsed time...
  current_time += tm_get_tick_delta();

  // Then, try to run all registered tasks
  for task in tasks:
     if task.ready
       task.run()
}

[annikaolson]

I've been trying to get the interrupts to work, since the actual C code changes are pretty simple. Unfortunately, the interrupts seem to not be triggering the ISR - I have, however, narrowed down the issue.

I added a second interrupt, and currently I have new mode interrupt at ID 61, and old mode at 62.

With this code, attached to 61 (or 62), nothing was happening. I put my current code into my testbench to simulate it, and the logic should be generating interrupts fine (both while the sensors are enabled and not enabled). When I comment out the function call to XScuGic_SetPriorityTriggerType, however, the interrupt is called. In fact, its called when I set the trigger (last arg) to 0 or 1, but not 2 or 3 (rising edge detection) which is strange.

When I set it to 0, 1, or comment the line out, the ISR is called at the same rate no matter what I change the user ratio to (I tried setting it really high since in the past I've done that to slow down the ISR call), so it's not actually calling it at the correct rate (its based on the user ratio - even when its based on all_done, still need to wait for the next trigger).

To make sure its actually detecting something from the interrupt, I made the old mode (ID 62) always set to 0 in the timing manager IP, and when I hooked up ID 62 to the initialization code, nothing happened. So its clearly detecting something for the interrupt, just not correctly it seems.

Edit: just set a breakpoint at the ISR and its actually being called when the timing manager is being initialized when I don't have the call to XScuGic_SetPriorityTriggerType, so not actually working correctly. Immediately after XScuGic_Enable(intc_instance_ptr, INTC_INTERRUPT_ID_0); is where its calling the ISR and getting stuck.

[annikaolson]

So this issue in the comment above came with this logic:

else if (~sched_source_mode & (count == user_ratio)) begin
            sched_isr <= 1;
end
else if (sched_source_mode & ~sensors_enabled & (count == user_ratio)) begin
            sched_isr <= 1;
end

The reason that it was getting stuck there and only calling the ISR when the trigger type was 1 (active high) was because it was detecting that count = user ratio before the interrupt was set up, since the PWM was already generating signals. So sched_isr was high, but it never detected a rising edge, never called the ISR, and never cleared it. So it just stayed high no matter what.

All I had to do was clear the ISR at the end of initializing the timing manager, setting the interrupt back to 0, and everything worked fine since now that the ISR is set up, it can correctly call it and clear the interrupt.

[annikaolson]

elapsed time each time scheduler called:

• SysTick timer (old method, replacing): (1/10kHz SysTick frequency) * 1,000,000 = SYS_TICK_USEC (period in usec)
• Mode 0 of new implementation:
  • (1 / (PWM frequency / user ratio)) * 1,000,000 = period in usec or
  • (#clock cycles between triggers [count_time] / 200 MHz FPGA freq) = period in usec
• Mode 1 of new implementation:
  • sensors not enabled: same as above
  • sensors are enabled: all_done stays high until the next trigger, so the period in usec will be:
    • time of the longest sensor for that cycle

will just be easiest to add a counter for the time in the fpga that counts the elapsed time, and is reset on sched_isr

[npetersen2]

Excellent work! Really nice to see this working in the lab this morning.

Going forward:

• @annikaolson will make a PR to merge these changes into v1.3-staging and we will review. Please add screenshots of the top-level .bd changes so @codecubepi can see the changes clearly.
• After review of just this content, we'll close this issue
• Once the PR is ready to go, @codecubepi will hand-merge these changes with his latest changes and make a new PR
• We'll review the full PR and get it merged
• Once @codecubepi combines everything (AMDS-related changes and the timing manager changes), we'll merge to v1.3-staging and then ping @anirudhupadhyaya that this is ready for testing on his motor set-up.

Today is @annikaolson last time in the lab until later, so if more testing needs to be done, we'll lean on @codecubepi . :)

[codecubepi]

Completed with merge of #397 (#392) into v1.3-staging.