wwarthen / RomWBW

System Software for Z80/Z180/Z280 Computers
GNU Affero General Public License v3.0
339 stars 99 forks source link

hbios - freertos - scz180 - access to z180 interrupt table #87

Closed feilipu closed 4 years ago

feilipu commented 4 years ago

I'm interested to use the PRT1 in the z180 to generate regular timing interrupts for my program. But I can't find in the documentation where the z180 interrupt address table is stored, or identify which (if any) HBIOS call is used to insert an interrupt handler into the z180 interrupt address table.

An alternative (perhaps supported?) would be to have the regular HBIOS tick (50Hz) call a function in my program on every tick. This would save consuming the PRT1 for essentially the same function that is provided by the HBIOS tick.

Any guidance on the best way to provide a timing interrupt within HBIOS? Or, without breaking HBIOS?

wwarthen commented 4 years ago

It is entirely possible to hook the existing Z180 timer interrupt in RomWBW via API calls. I will provide some better instructions as soon as I have a bit more time.

wwarthen commented 4 years ago

There is an API in RomWBW HBIOS for interrupt hooking. It is documented (albeit poorly) in the RomWBW Architecture document. It is the SYSINT function ($FC). This is at the very end of the document.

All of the standard Z180 configs are IM2 based. If you look at hbios.asm, search for HBX_IVT. This is the interrupt vector table. Just above it is a comment that documents the interrupt slots assigned to the standard interrupts. In your case, you are going to hook slot 2 (TIM0) which is used for all Z180's as a 50Hz timer.

There is a good example of hooking the vectors in the inttest.asm application included with RomWBW. The app is more generic than you need, but I think you will easily see the simple routines for hooking and unhooking interrupt handlers.

The new interrupt handler must be in the top 32K of memory.

I'm hoping this is enough to get you started. Let me know how you do and ask questions as needed.

Thanks,

Wayne

feilipu commented 4 years ago

Thanks. I've had a look around in the code. From what I see the Z180 address table entries are all pointing at a HBX_INT with a following code byte to route the interrupt onwards via the HB_IVT. I think the SYSINT function writes to the HB_IVT table which means an interrupt handler will be invoked after the HBIOS partial context save.

I think the best thing for me to do is to use the PRT1 for timing, and then hook the handler directly into the Z180 interrupt address table. This will allow me to set the timer interval at any rate that the user needs. Also, I need to use a full context save including alternate registers and interrupt state because the task switching can be initiated through various mechanisms, not solely the timer interrupt.

So, I need to understand where the Z180 interrupt address table is located, to write the handler address directly. Is that address a supportable (fixed) location?

But, to not break HBIOS, I'll have to ensure that HBIOS functions have not banked the memory when the interrupt is called. So the handler has to look something like this.

prt1_isr() __naked
{
    portSAVE_CONTEXT_IN_ISR();
    xTaskIncrementTick();
    // read some location to confirm we're not banked (in HBIOS)
    // if banked then restore context and return
    // otherwise
    vTaskSwitchContext();
    portRESTORE_CONTEXT_IN_ISR();
}

So, a further question. Is the memory banking address readable generally on all platforms? Or as a better question, is there something else unique that would be preferable / supportable that I can use to determine if a HBIOS function has the 0 bank switched in?

wwarthen commented 4 years ago

Sure, you can certainly just put entries directly in the HBX_IVT table. Yes, in this scenario, you will be entirely responsible for saving and restoring context, not to mention acknowledging the interrupt and using RETI & EI at the end of the interrupt processing.

The Z180 interrupt address table (HBX_IVT) will always be found at 0xFF00. This does not change, so you are safe to assume it is there. RomWBW programs the Z180 at boot such that the start of the interrupt vectors is at 0xFF00. This fixed and you are safe to assume it is there as long as the processor is using interrupt mode 2 which is the norm for all Z180 platforms. PRT1 will therefore be found at slot 3 of the HBX_IVT.

The IVT and your handler must be located in the top 32K or RAM because an interrupt may occur when various banks are selected in low RAM.

I am a little confused about what you want your ISR to do. You can indeed determine the currently selected bank by examining the value at 0xFFE0. This is the current bank id. However, this is a tricky thing to do because a bank switch can be "in progress" when an interrupt occurs. If you look at HBX_INT, you will see how RomWBW safely handles the selection of the HBIOS bank at the time of an interrupt and restores the original bank at completion. It is easier for your ISR to just set flags/counters in high ram and then return. Then your mainline code would just refer to these flags/counters as needed.

Not sure if I am helping here. I may need to better understand the intended functionality of your ISR.

You may want to reconsider using the API to accomplish this. If you use the API, the context saving is handled for you and the HBIOS bank is guaranteed to be selected in low RAM when your ISR gets control. You can still use PRT1 as desired.

Thanks,

Wayne

feilipu commented 4 years ago

Sure, you can certainly just put entries directly in the HBX_IVT table. The Z180 interrupt address table (HBX_IVT) will always be found at 0xFF00.

Noted. Excellent!

The IVT and your handler must be located in the top 32K or RAM because an interrupt may occur when various banks are selected in low RAM.

Noted.

I am a little confused about what you want your ISR to do. You can indeed determine the currently selected bank by examining the value at 0xFFE0. This is the current bank id. However, this is a tricky thing to do because a bank switch can be "in progress" when an interrupt occurs.

Yes, I see the HB_CURBNK is set with the currently running bank, and that it is maintained through HBIOS interrupt handling, even when running in the BID_BIOS bank. I guess the only sure fire way to know where the code is running from is to read the Z180_BBR value. That's OK, and I'll just go with that.

If you look at HBX_INT, you will see how RomWBW safely handles the selection of the HBIOS bank at the time of an interrupt and restores the original bank at completion. Not sure if I am helping here. I may need to better understand the intended functionality of your ISR.

The ISR is a part of freeRTOS with preemptive scheduling. But it also needs to handle cooperative scheduling too, where a task can voluntarily yield with interrupts in a specific state, and the same state needs to be provided to the task when it is scheduled back in.

Since the task control block maintains a full CPU state stacked (including interrupt status and alternate registers), it won't be possible to use the HBIOS HBX_INT code. The state stored on the stack in the task control block needs to be identically returned both following a cooperative and a preemptive task swap.

But, the HBX_INT code isn't identical, because it wasn't designed to be identical. It doesn't save IX or any of the alternate registers, for example. I guess it would be possible to store / return the missing registers in the right order, and have the cooperative yield match the same register stacking order. I'll look at possibly doing that.

The issue I see left is that a correct (but fake) interrupt status needs to be pushed onto the stack by the interrupt handler first, (following AF) but the HBX_INT code has already pushed a number of registers onto the stack before the interrupt handler gets invoked.

So my nominal code looks like this. If HBIOS is doing something, and I've interrupted it, then simply increment the tick and exit. HBIOS won't know, and the RTOS doesn't care...

prt1_isr() __naked
{
    portSAVE_CONTEXT_IN_ISR();
    xTaskIncrementTick();
    if( __io_Z180_BBR != BANK_0 )
        vTaskSwitchContext();
    portRESTORE_CONTEXT_IN_ISR();
}

Some more things to think through. But, I think I've got a working direction now. Probably some questions, when things aren't working. 😄

wwarthen commented 4 years ago

Sounds like a serious challenge. Hope it goes well.

feilipu commented 4 years ago

Just got FreeRTOS running on YAZ180, :+1: So I'm putting HBIOS on the list to finish on the weekend. :smile:

wwarthen commented 4 years ago

Excellent! Looking forward to the progress report.

feilipu commented 4 years ago

Ok. Progress report.

TLDR: FreeRTOS now works on SCZ180 with HBIOS. :+1:

But, there is still something wrong... because it only runs for about 2 minutes, before I get a panic. But, running for 2 minutes means that the code is mostly OK.

Can I check some assumptions, please?

On not being in the HBIOS bank I do a test: BBR != 0xE000.

If that is true then I assume current execution is not in HBIOS, which is running in the 32k Bank at 0xE0000 Is that a correct assumption?

I think there may be more to this, because this is the only place where a problem could eventuate.

I need to copy the timer_isr() up to somewhere safe. So I've elected to put it at 0xFB00, as that is in the slack space after dbgmon. It is less than about 0x50 Bytes, so could go pretty much anywhere. Would you suggest another (better) location, where it can be safe?

wwarthen commented 4 years ago

TLDR: FreeRTOS now works on SCZ180 with HBIOS. 👍

Awesome!

But, there is still something wrong... because it only runs for about 2 minutes, before I get a panic. But, running for 2 minutes means that the code is mostly OK.

I assume it is not exactly the same length of time consistently, right?

On not being in the HBIOS bank I do a test: BBR != 0xE000.

If that is true then I assume current execution is not in HBIOS, which is running in the 32k Bank at 0xE0000 Is that a correct assumption?

Hmmm... BBR is an 8 bit register. It will have the value 0xE8 when the HBIOS bank is selected. It will have the value 0xF0 when the user bank is selected. It seems like testing for BBR != 0xE8 should work.

Your ISR must be in upper 32K of CPU address space at all times. HBIOS switches to a small temporary stack before selecting the HBIOS bank. If your ISR hits at this point, you could overrun the stack.

I need to copy the timer_isr() up to somewhere safe. So I've elected to put it at 0xFB00, as that is in the slack space after dbgmon. It is less than about 0x50 Bytes, so could go pretty much anywhere. Would you suggest another (better) location, where it can be safe?

That seems good to me.

feilipu commented 4 years ago

Hmmm... BBR is an 8 bit register. It will have the value 0xE8 when the HBIOS bank is selected. It will have the value 0xF0 when the user bank is selected.

Hmm. I was going with 0xE0 because HBIOS bank was the lowest of 4 banks at the top of RAM. That’s what I think the architecture doc implies. As that is wrong, then it’s clearly the cause of the panic.

I’ll try again avoiding 0xE8 instead.

EDIT just realised they are 32kB Banks, and now I get it. And that’s why it is good to ask.

feilipu commented 4 years ago

BBR is an 8 bit register. It will have the value 0xE8 when the HBIOS bank is selected. It will have the value 0xF0 when the user bank is selected. It seems like testing for BBR != 0xE8 should work.

I decided that it was much better to test for the presence of the TPA at 0xF0, rather than absence of HBIOS and since the vTaskSwitchContext() is only present in the TPA.

So, now it works 100% FreeRTOS is available for SCZ180 HBIOS. EDIT 99%. Job nearly done.

Other HBIOS platforms can be added by simply choosing a Timer and configuring it, and adjusting the memory mapping for the TPA (only if needed). All done here.

But, one final question. If I was going to place a 0x50 Byte ISR when CP/M/HBIOS was running, where would be a logical place? I'm pretty sure that 0xFB00 won't work. Is there a slack space somewhere above 0x8000 that you can recommend?

feilipu commented 4 years ago

Oops. An update. Leave it running long enough, and it will eventually crash.

I think it is caused by a conflict in using the Serial ISR (if that needs to go into HBIOS bank). Not totally sure of why this is an issue (very occasionally), because the Timer ISR should back off if it detects it has triggered during a non TPA bank presence.

It would be easy to work around using a semaphore (in FreeRTOS) to protect the serial port. But, that shouldn't be necessary (and it should ideally "just work").

wwarthen commented 4 years ago

Hmmm... Is FreeRTOS doing any bank switching on it's own? If it does a bank switch and a serial int occurs, the serial ISR will restore the wrong bank at exit. If FreeRTOS is leaving the CPU memory space alone, I'm not sure what is happening. Interrupts are turned off throughout the entire time the HBIOS bank is swapped in during ISR processing.

feilipu commented 4 years ago

I'm pretty sure I know the cause. I forgot that xTaskIncrementTick() is also not above 0x8000, so it needs to be protected by a BBR check too.

Fortunately, there's a variable xPendedTicks exactly for this situation, so that if I can't process a tick increment, I simply need to increment this variable, and carry on in HBIOS.

EDIT using xPendedTicks was not necessary after stack adjustments.

Working on it tonight.

feilipu commented 4 years ago

Spent quite a few hours on this over the weekend, and still no definitive fix.

Did realise that pxCurrentTCB needs to be moved be uninitialised (in BSS which is above 0x8000), so that the ISR context save is being done correctly. Fixing this takes the average run time up to 15 minutes. But it will still crash in the end.

After thinking about it, I'm getting the feeling that it is a stack overrun. The context save needs 22 Bytes, so if the PRT1 interrupt handler is called when this stack is not available, then at this time there will be a problem. I need to look at HBIOS code some more to see if there is any time when interrupts are enabled, but the stack is not able to accept this context save.

wwarthen commented 4 years ago

After thinking about it, I'm getting the feeling that it is a stack overrun. The context save needs 22 Bytes, so if the PRT1 interrupt handler is called when this stack is not available, then at this time there will be a problem. I need to look at HBIOS code some more to see if there is any time when interrupts are enabled, but the stack is not able to accept this context save.

Quite possible. When an HBIOS function is called (e.g., read serial port), the upper memory proxy code switches to a small (20 byte) temporary stack (HBX_TMPSTK) just long enough to swap in the HBIOS bank. After that, the stack is switched to a much bigger stack in the HBIOS bank for the remainder of the HBIOS processing. The HBX_TMPSTK is also used briefly during the transition back from the HBIOS function call.

When HBIOS handles an interrupt, the first thing it does is switch to a private stack for interrupt handling, so HBX_TMPSTK size is not an issue. It sounds like your ISR needs 22 bytes. So, if the interrupt hits inside of a few instructions inside HBX_INVOKE, you are probably screwed.

You could probably increase the size of HBX_TMPSTK by 4-6 bytes without causing any issues.

Good luck!

-Wayne

feilipu commented 4 years ago

When an HBIOS function is called (e.g., read serial port), the upper memory proxy code switches to a small (20 byte) temporary stack (HBX_TMPSTK) just long enough to swap in the HBIOS bank. The HBX_TMPSTK is also used briefly during the transition back from the HBIOS function call.

Not sure this is my issue, but just looking at the HBIOS code, I'm wondering whether slightly reorganising the use of the various stacks would help to simplify any issues going forward?

Current state for the discussion:

There's 20 Bytes then HBX_TMPSTK then (in my build for SCZ180) 38 Bytes free HBIOS PROXY STACK space: 38 bytes which is used as stack by interrupts only as HBX_STK.

Then later (above 0xFF00) there's the HBX_INTFILL which for my compile is HBIOS INT space remaining: 20 bytes, followed by a 64 Byte HBX_BUF inter-bank copy buffer.

Suggestion for @feilipu:

I should use the HBX_STK stack specifically for my interrupt, rather than relying on the stack to be somewhere safe. This space would be unchallenged (as two interrupts shouldn't fire at the same time), and is large enough (38 Bytes) not to cause problems. This would add a configuration as this specific stack location would be different depending on the specific implementation. But, not a big deal.

Suggestion below would make the HBX_STK space even more usable at 58 Bytes.

EDIT further thought reveals that this was an uninformed suggestion. It isn't possible to proceed with a context switch if the context isn't pushed onto the exiting Task's stack.

Suggestion for @wwarthen:

Would it be useful to consider moving the HBX_TMPSTK to the 20 Bytes before HBX_BUF?

That is not a space needed for growth or flexibility, and it would provide the HBX_STK and the HBIOS proxy with more uncontested space. By that I mean if any interrupt processing needs a bigger stack than 38 Bytes, and it happened during processing of a HBIOS API call, then it would overwrite the HBX_TMPSTK. Boom.

HBX_TMPSTK doesn't look like it needs the whole 20 Bytes, as the only functions called are HBX_BNKSEL, and HBX_PEEK, and HBX_POKE which don't have much stack usage (except CALL RET).

Also, the size of HBX_STK is not a fixed amount as it is sized by a remaining space calculation. A change in the HBIOS proxy code would consume free stack, possibly leading to something unrelated blowing up, and be potentially difficult to triage. Adding 20 Bytes to it doesn't fix this issue, but does add some more headroom.

feilipu commented 4 years ago

A situation where the more thought is applied, the more nuanced it gets.

I'm reminded that the real reason that the timer interrupt saves the entire CPU context, and then a reference to it in pxCurrentTCB, is that its job is to switch between multiple task contexts. So saving references to a temporary stack not associated with (any) task is going to lead to a crash.

Then, as well as confirming that the BBR is correct, I also need to confirm that the current stack is not inside the HBIOS memory stubs (above 0xFE00) or, more robustly, is outside the configured application stack and heap.

But, because the context handling has to be identical for cooperative and preemptive context switch, I need to make sure that any stack I'm using is more than 22 Bytes plus stack for handling the context switch decision.

Which means that even the HBX_TMPSTK stack has to be large enough not to overrun. Or, alternatively disable interrupts for the short period while the bank is being switched and HBX_TMPSTK is being used. I think that might be the best way given there's not enough space available...

So with the HB_DI HB_EI protection around the HB_INVOKE temporary stack usage, #96, the problem is solved. FreeRTOS is now running stably.

I've still some work to do to ensure that I don't lose ticks when the HBIOS Bank is selected, but that's no longer a HBIOS issue.

wwarthen commented 4 years ago

Well, it is great to hear that you got it stable. There is a downside to using EI/DI inside of HB_INVOKE. It means that code using HBIOS functions cannot keep interrupts disabled across HBIOS calls. I think there are already a few places where this would be an issue.

I guess the RTOS timer ISR must be using more than the 20 byte stack available at HBX_TMPSTK. Is there any way the RTOS timer ISR could switch to it's own stack during interrupt processing? If not, I will review the implications of using DI/EI in HB_INVOKE, but would take a little bit of time.

feilipu commented 4 years ago

There is a downside to using EI/DI inside of HB_INVOKE. It means that code using HBIOS functions cannot keep interrupts disabled across HBIOS calls.

I originally tried to write an interrupt preserving patch but, long thought process - short, the only way I could see to make it work was to use the AF' register to cache the function return whilst restoring the interrupt state, and that might have had other implications that I wasn't aware of.

So to check that this really was a solution (and it certainly seems to be), I just did it simply with DI/EI.

I guess the RTOS timer ISR must be using more than the 20 byte stack available at HBX_TMPSTK.

I'm not sure I'm the best person to illuminate the mechanics of freeRTOS, but here goes... When running the Scheduler assumes that each Task is running with its own stack (or context). The only way that the stack and context changes is if a) the Timer ISR initiates a Yield preemptively, or b) if the Task itself initiates a Yield cooperatively.

So during the context swap the entire state of the CPU must be captured. This includes the state of the IFF2 register, and all alternate registers. I make that to be 22 Bytes, plus the ISR return address pushed by the interrupt. Lets say 24 Bytes. All of that is assumed to be stored on the bottom of the stack of the currently running Task, and this stack pointer is written finally to the currently running Task Control Block (TCB) pointer.

After the context swap (which is a function which also uses stack from the running Task until it returns), with a new TCB pointer containing the new stack pointer, which is then used to unroll the CPU state and return from the ISR / Yield to the newly restored Task.

The key point is that there is already one SP change during the Timer ISR. The context save and restore are assumed to be from different Task stacks. And also the restore within the preemptive Timer ISR may have been originated by a cooperative Yield. So the context stack has to be identical for both mechanisms.

Is there any way the RTOS timer ISR could switch to it's own stack during interrupt processing?

Its complicated. I spent some time thinking about how to do that. I wasn't able to think though a clean solution. You'd have to be aware which Task was running when the ISR was triggered, and move the CPU state over to that Task's stack, all while trying to be responsive inside an ISR.

The "best" solution I have is to go into the ISR, then identify the Bank and if it is wrong (in HBIOS) back off without doing the Tick increment (a function using stack), and without doing the Yield.

[Later I need to correctly increment the missed Tick counter, as the normal Tick increment function shuffles the waiting Task list to put pended Tasks in the ready to run list, and organises the Event list too. It can also process missed Ticks to jump the Tick counter forward. This way "real time" doesn't slip.]

If not, I will review the implications of using DI/EI in HB_INVOKE, but would take a little bit of time.

The other way, which may be just as hard, is to ensure that there is always enough space on the currently enabled stack. Then the Tick ISR can understand where it is, and if the Bank or Stack is wrong it can then back off (after incrementing the missed Tick counter), unroll its context, and return.

I couldn't find enough free space in the HBIOS memory layout to do this properly, so I abandoned that thought. I also thought about perhaps one of the buffers could be used? But again I don't know HBIOS well enough to know what could be changed.

Anyway. Two workable solutions being; DI/EI over the small temporary stack, or make the temporary stack (say) 32 Bytes. Either would work.

wwarthen commented 4 years ago

OK, I get it. Give me a little time to work through this.

Thanks, Wayne

wwarthen commented 4 years ago

OK, how about 64 bytes? :-)

I just checked in a change that reuses the proxy bounce buffer as the temporary stack. The uses are mutually exclusive so it is a win-win. Brief testing looks good for me.

-Wayne

feilipu commented 4 years ago

Yes. that will work. 👍

EDIT Didn't work. 😞

I wasn't sure that they were mutually exclusive, as I guessed the bounce buffer might be used to pass info on the way out of the invoked function.

As it isn't; great solution.

Removes the TMPSTK which gives the interrupt stack more space. And the bounce buffer has also space below it that can be used if the new temporary stack happens to grow larger than 64 Bytes.

wwarthen commented 4 years ago

My (undocumented) rule is that a caller should provide required buffer space and that the bounce buffer is purely for moving the code between banks. So, as long as I stick to my guns, the bounce buffer is fair game.

feilipu commented 4 years ago

I've done some testing, and the instability is back. I'm getting between 5000 Ticks and 100,000+ Ticks before the crash, but it comes inevitably.

Before any more HBIOS modifications, let me rule out the xIncrementTick() function blowing the whole 60 Bytes available to it (after the context save, and using the slack space) first.

Wait! Actually, I know what the problem will be. Attempting to Yield when using the TMPSTK stack will be the issue (now that it is not simply blowing up on the stack, and the stack is unguarded). I'll work on that this evening.

wwarthen commented 4 years ago

Wait! Actually, I know what the problem will be. Attempting to Yield when using the TMPSTK will be the issue (now that it is not simply blowing up on the stack, and the stack is unguarded). I'll work on that this evening.

I hope that is it because I don't think I can find any more stack space! :-)

feilipu commented 4 years ago

Excellent. FreeRTOS is now 100% reliable.

With the additional stack available I don't have to take special care with xIncrementTick(), other than make sure the bank is set to user TPA, which is a big help.

Simply ensuring the stack is not in the 0XFFnn page, along with the user TPA bank check, is sufficient to ensure that vTaskSwitchContext() only happens from within the running Task.

For interested readers the configurations specific to the SCZ180 target (SC126, SC130, and upcoming SC131), are found from in FreeRTOSBoardDefs.h .

I've opened #99 to move all the HIBOS temporary stacks up into 0xFF00 memory.

This is partially to clean up the use of the interrupt stack. But, my ulterior motive is to make testing for their usage easy because they will all be in the same page of RAM, so I'll only have to test for 0xFFnn.

I'm OK to force an upgrade to latest RomWBW version before using FreeRTOS, and this will be the trigger for that requirement.

I think we're done here. ;-)

wwarthen commented 4 years ago

I have merged #99. I did check-in a further update that changes the naming conventions somewhat with the hope of making everything a little easier to follow (not sure if I did or not).

Once you confirm my latest check-in to hbios.asm is OK, I will close this issue.

Thanks!

Wayne

feilipu commented 4 years ago

I had a quick look at your further hbios.asm update, and thanks for including the note about freeRTOS.

In #99 I was unsure whether PEEK and POKE (and for that matter BNKCPY) were invoked through the HBIOS API at all. So, I moved their stacks into the same buffer as INVOKE as a precaution (although for BNKCPY could not be so).

If PEEK, POKE, and BNKCPY are only used by DBGMON or during the initial loading process (not by the API) then there won't be an issue. And its all good. 👍

wwarthen commented 4 years ago

So... PEEK, POKE, and BNKCPY can all be invoked through the HBIOS API. However, I am not quite following your concern though.

For example, if PEEK is invoked through the HBIOS API, then HB_INVOKE will use TMPSTK briefly at the start and end of the routine while switching the bank. TMPSTK is not used during the actual HBIOS processing though (HBIOS has it's own low memory stack in it's bank).

Here's what I think should happen:

    - HBX_INVOKE uses and releases TMPSTK to activate HBIOS bank
    - HBX_INVOKE switches to HB_STACK (a low memory HBIOS stack) to run the PEEK API
        - PEEK API calls HBX_PEEK in high memory
            - HBX_PEEK uses and releases TMPSTK during processing
        - PEEK API returns to HBX_INVOKE
    - HBX_INVOKE uses and releases TMPSTK to restore callers bank
    - HBX_INVOKE restores callers stack
    - HBX_INVOKE returns to caller

I don't think there is any overlap in the use of TMPSTK.

Do you think I am missing something here? I'm worried I have not understood you.

Thanks!

Wayne

wwarthen commented 4 years ago

Oops, correction. HBX_INVOKE now uses HBX_BUF instead of TMPSTK for temporary stack.

    - HBX_INVOKE uses and releases HBX_BUF to activate HBIOS bank
    - HBX_INVOKE switches to HB_STACK (a low memory HBIOS stack) to run the PEEK API
        - PEEK API calls HBX_PEEK in high memory
            - HBX_PEEK uses and releases TMPSTK during processing
        - PEEK API returns to HBX_INVOKE
    - HBX_INVOKE uses and releases HBX_BUF to restore callers bank
    - HBX_INVOKE restores callers stack
    - HBX_INVOKE returns to caller
feilipu commented 4 years ago

If the user calls the HBIOS API, while freeRTOS is running, we have to assume that the Tick interrupt will happen anytime (at the worst possible time). I would prefer not to caveat the HBIOS API to users, and I'm sure you feel the same.

This means that the active stack can never be too small to handle a full context save and xIncrementTick() otherwise-> crash.

So I guess PEEK and POKE need to use the big stack.

feilipu commented 4 years ago

Just thinking a bit further... perhaps PEEK, POKE, and BNKCPY could use DI/EI to protect their use of the small stack (inside the API call?

HBX_PEEK, HBX_POKE, HBX_BNKCPY, could preserve the interrupt state on the HB_STACK, disable interrupts and use the small stack, then recover the former interrupt state off the HB_STACK when they return to INVOKE.

wwarthen commented 4 years ago

Ahhh... now I understand. I honestly didn't think that such calls would be occurring under FreeRTOS, but that was stupid -- why wouldn't a FreeRTOS app use them.

PEEK and POKE are not an issue (they can use use HBX_BUF). However, BNKCPY is a big problem because it cannot use HBX_BUF for stack. I think this was also an issue in your last update as well because you were using HBX_BNKSTK (20 bytes), which is not enough for FreeRTOS, right?

-Wayne

wwarthen commented 4 years ago

Just thinking a bit further... perhaps PEEK, POKE, and BNKCPY could use DI/EI to protect their use of the small stack (inside the API call?

HBX_PEEK, HBX_POKE, HBX_BNKCPY, could preserve the interrupt state on the HB_STACK, disable interrupts and use the small stack, then recover the former interrupt state off the HB_STACK when they return to INVOKE.

Yeah, I was just thinking about that too. That would be a pretty clean solution. I have this dim memory that there is a scenario where reading the current int state can fail. I need to refresh my memory on this. Maybe you know what I am talking about?

feilipu commented 4 years ago

Yes. I didn't have the HBX_BNKCPY fixed in #99. But, I didn't really know whether it was a problem (now you've confirmed it is).

My suggestion: just to get HBX_PEEK, and HBX_POKE to use the buffer as stack. And wrap only HBX_BNKCPY in a state preserving DI/EI solution. Best not to add useless bytes for interrupt management where they're not needed.

feilipu commented 4 years ago

On reading the interrupt state. There is a bug which only affects NMOS Z80 devices. z88dk has a CPU type flag that switches different solutions depending on the target (because for z88dk, the target may be 40 years old).

The normal CMOS Z80 solution looks like this (which I use in freeRTOS critical sections)

ld a,i
di
push af
...
code here running with interrupts disabled
...
pop af
di      ; < - optional, only needed if somehow interrupts could become re-enabled (via NMI).
jp PO,$+4 ; < - recovers the IFF2 state. PE if interrupt status was enabled.
ei
        ; < - normally ret, but interrupts enabled AFTER any instruction.
wwarthen commented 4 years ago

Ah, cool. If it is only NMOS processors, I can live this this!

I will make this change later tonight. Thanks!!!

wwarthen commented 4 years ago

OK, I just checked in the interrupt protection for PEEK, POKE, and BNKCPY functions. It is a little hard to test, but seems to be working in general.

As far as I know, this should complete the FreeRTOS needs.

Thanks!

Wayne

wwarthen commented 4 years ago

@feilipu, if you think we are ready, I will bump the RomWBW version numbers and issue a full pre-release. This will allow you to tell folks exactly which pre-release they will need in order to run FreeRTOS. Just let me know if you are ready for this.

-Wayne

feilipu commented 4 years ago

You decided to go with wrapped rather than moving buffers again? It looks good. Let me test tonight.

wwarthen commented 4 years ago

Yes, since you provided code that allows the interrupt state to be maintained across the call, that works for me.

On Tue, Mar 3, 2020 at 8:48 PM Phillip Stevens notifications@github.com wrote:

Still business hours here.

But you decided to go with wrapped rather than moving buffets again?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/wwarthen/RomWBW/issues/87?email_source=notifications&email_token=AATNT2J4GRHDYGIXNJN7VTDRFXMQ3A5CNFSM4K2CCLT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENWK3OI#issuecomment-594324921, or unsubscribe https://github.com/notifications/unsubscribe-auth/AATNT2IMAG7ZNHXEO4CQLM3RFXMQ3ANCNFSM4K2CCLTQ .

feilipu commented 4 years ago

I've just checked the hbios.com code, and I think there are two further HBX_BNKCPY instances that need to be protected to enable safe access to the HBIOS DIO API.

In HB_DSKWRITE here. In HB_DSKREAD here.

The final instance is in the startup code, so I can't see that being a problem. 👍

Reading deeper, the other issue I see is HB_BNKCPY is used in quite a few places through the code base. Rather than wrapping every call of HB_BNKCPY, it might be easier to preserve the interrupt status within HBX_BNKCPY itself. Since A register is not used for its call, it doesn't need to be preserved.

Did a little PR #100 to address this. I'll continue testing on this fix. Updates in a few hours.

feilipu commented 4 years ago

OK. I've uploaded a little video, showing the results of our small endeavours...

I'm hammering the DIO API with the SD (also tested RAM disk). No errors in reading or writing. That is all happening whilst being interrupted 256 times per second, each time with a Tick increment and a Yield decision.

Simultaneously, there's a small Task running (making the total of 3 Tasks including the IdleTask that is run when no other Task is on the ReadyList). It won't interrupt the diskio because it is running at a lower priority. You can see the second Task running when the diskio testing is paused between runs.

Very occasionally are still some glitches. I don't know what's left to fix. They might even be caused by glitching from the FTDI interface if the USB interface drops.

Anyway, I think with #100 we're done, done. Ship it! 🍾

EDIT Nope... not done. But getting close.

wrljet commented 4 years ago

Ah, cool. If it is only NMOS processors, I can live this this!

After that comment and info about the interrupt difference/bug, I spent two hours on the phone in bed reading up on all that stuff on the web. :-)

feilipu commented 4 years ago

I am going to move the DI/EI protection for PEEK and POKE back into the main HBIOS code.

If space is an issue for N8, then don’t do the DI/Ei. Rather just use the bounce buffer as stack, like INVOKE.

Secondly, I want to confirm how you are checking that HBIOS is active. The first thing that HBX_INVOKE does is save the callers SP. If an interrupt fires immediately after this and RTOS does a context switch, the new context could call HBX_INVOKE and clobber the SP value stored by the original context, right?

I missed that issue. You’re right. Explains the last few glitches.

A flag won’t be sufficient. We need a SRA mutex. Same, but different. Code example to follow.

wwarthen commented 4 years ago

Your last comment showed up just after my last check in.

Yes, I will switch PEEK/POKE to use the bounce buffer instead of DI/EI. Better solution as long as PEEK/POKE are never invoked directly, but only through HBIOS API (which is a reasonable assumption).

I will wait for your code example and try to implement SRA mutex. Worried about code size again. Sigh.

wwarthen commented 4 years ago

Ah, cool. If it is only NMOS processors, I can live this this!

After that comment and info about the interrupt difference/bug, I spent two hours on the phone in bed reading up on all that stuff on the web. :-)

Hi Bill!

Yeah, we have had some serious fun working through this FreeRTOS adaptation. Phillip has taught me several nice new tricks in the process. It has brought to light some serious deficiencies in RomWBW that I am happy to have ferreted out. I think we are close to something final. :-)

wrljet commented 4 years ago

Wayne, Earlier in this exercise I was thinking that just making stacks bigger -hoping- they work is not a way to ensure success with this stuff. But I see now things are really much improved with the new layout. Bill