Optimization Assembly Problems

I think it is an atomicity issue that show up when the timing changes due to optimization.

Erik

My only objection to that is when I am testing this code, I am hardcoding values into those variables and still having the issue. I think you might still be right about me having a potential issue with atomicity but I think I might still have some other issue directly related to storing DiffPressure1 into DigPressure1. Or could that still be an atomicity issue, even with hardcoded values?

Your disassembly screenshot shows that the code contains absolute register accesses. IIRC you have to be careful with assigning register banks to ISR's. Or I could be wrong and this is handled automatically by the compiler. Anyway, you can test this theory by ticking 'Don't use absolute register accesses' checkbox in compiler settings. Of course, less efficient code will be generated because of that.

My only objection to that is when I am testing this code, I am hardcoding values into those variables and still having the issue.
when doing so, do you disable the interrupt associated with In CommBuff ISR:

I do not but ISRs must be directly called when in simulation mode so I assume variable values would not be updating at the same time a variable is being called on (maybe this is a false assumption). Or is my understanding of the term "atomicity" incorrect? (if I understand it correctly, it's having a value called while it's being updated, causing incorrect values to be read in).

There are other ways I could approach this, with the CommBUff ISR collecting the commData and then using that data external to the CommBuff ISR. It would cause a delay in communication (since my program would have to execute before checking the value) and there is a possibility a previous piece of data could be overwritten before it's handled (though I could place all input values into a queue or array). I'm not sure if this would be a better approach (I guess if it solves my issue, it is). I still have trouble wrapping my head around why it would be an atomicity issue when in simulation mode though (once again, assuming my understanding is correct).

For all intents and purposes, this is the call I am using to call the CommBuff ISR:

/*I have a check that finds a value saturated and outputs this value */
DiffPressure1 = 0x0FF8;
COMBUF = 'a';
commBuff_ISR();

So the value of DiffPressure1 is definitely stored prior to the commBuff ISR being called.

An ISR should never be called directly.

The ISR should be activated by a hardware event of some kind.

Some interrupt sources will trig automatically when simulating - for example timer interrupts, ADC conversion done interrupts etc. Some interrupts requires that you have a simulation script to trig the event - such as changing a state on a processor pin, or sending a character to the UART.

Atomicity issues happens when the main loop reads one byte of a multi-byte variable, and an interrupt happens and the interrupt handler modifies this multi-byte variable. When the interrupt ends, the main loop will continue with the read. So some bytes was read before the update and some after the update.

Same thing can happen when the processor have a timer or similar with a 16-bit counter that ticks continuously, and the code reads these two bytes in the wrong order in relation to how the timer was designed. Then the counter can tick from 0x12ff to 0x1300 and the main loop either got 0x1200 or 0x13ff. Note that the 16-bit peripherial registers have a design so when the 8-bit code reads a specific "first" byte, the hardware latches the other byte into a special latch to make sure the two 8-bit reads will catch a correct 16-bit value. Reading in wrong order makes this behind-the-scenes latch functionality fail.

So atomicity issues comes from variables being updated asynchronously, in relation to the code that want to use the variables. Interrupts are asynchronous in relation to the main loop. Peripherial hardware registers are asynchronous to both main loop and interrupt handlers.

Atomicity isn't just relating to multiple reads to get the full content of a larger variable. It can also happen from read/modify/write situations. The code loads the variable (potentially atomic) and then modifies the value and writes back the changed value.

If the main loop performs a load, and the code then gets interrupted, then the ISR can also do a read+modify+write before returning to main. So main then do an update of a value that is already non-current and then writes back a broken value.

Thanks for the information on atomicity. I believe I am handling this correctly as when I receive a 'a' character, I am updating the values into temp destinations (such as DigPressure1) so if the other variables are updated, it won't affect the pressure values I took at that time. I don't modify DiffPressure1 or Pressure1 though so I don't believe I will cause any value disruptions.

Regarding the direct calls to the ISR, I only do that in simulation mode (as you said, ISRs should be handled by hardware). Since there is no hardware in simulation mode though, the code won't execute if the ISRs are not called (since a majority of my code is interrupt/flag driven in that code won't execute until a variable is updated and a bit is set). In simulation mode, I just directly call the ISRs because I don't know any other way to simulate a hardware call (up til now, it hasn't caused any issues).

Don't worry, I rely on hardware for ISRs once they're programmed on a chip!

Since there is no hardware in simulation mode though, the code won't execute if the ISRs are not called (since a majority of my code is interrupt/flag driven in that code won't execute until a variable is updated and a bit is set). In simulation mode, I just directly call the ISRs because I don't know any other way to simulate a hardware call (up til now, it hasn't caused any issues).

You're grievously underestimating this simulator. Keil has simulations for just about all peripherals of just about all known '51 derivates, including fully simulated interrupts.

Calling IRQ functions directly really is a bad idea, if only because it fails to simulate the most important aspect that differentiates interrupts from normal code flow: the fact that interrupts can and will happen anywhere. And of course the registers of the peripherals that those interrupts are supposed to have come from will be in the wrong state, so the interrupts themselves can't even work properly.

You end up with a simulated program that is wildly different from the real thing, rendering any results of such simulation suspect, and many of them useless.

Wow, if there's a way to simulate hardware interrupts, you're right, that would absolutely be the best way to approach it. I didn't realize Keil had these capabilities. Is it pretty easy to setup?

There was a reason why I wrote "Some interrupt sources will trig automatically when simulating - for example timer interrupts, ADC conversion done interrupts etc. Some interrupts requires that you have a simulation script to trig the event - such as changing a state on a processor pin, or sending a character to the UART."

You need to play with stimuli manually or using scripts to fake external interrupts.

But for internal devices, the simulator handles them just as normal.

Your code initializes the ADC and tells it to convert. The debugger script can then fake values to the ADC and your ADC ISR will pick up the values the debugger script supplied. The script can generate noise, or ramps or whatever you want.

So you really should simulate as much as possible of the complete system - allows you to test the code long before you get first prototype hardware.

This makes sense. Are you guys saying that by simply calling the ISR directly as I am in my simulation, I am creating potential issues? It seems to me that there clearly is a problem storing DiffPressure1 into DigPressure1 since it is placing a 0x0097 in DigPressure1 rather than the 0x0FF8 in DiffPressure1. I don't believe the simulation is creating an issue here (especially since I am seeing the same kind of thing with it burned directly to a chip).

I do agree that when simulated on an actual circuit, there may be things I would normally miss because of timers but in this particular case, it looks like something specific is happening here. It sounds like it's pointing to source code to me.

And looking back over the previous suggestions (because someone mentioned the disassembly code), Mike hit it right on the head, it was assigning an absolute address. I checked the "don't use absolute addresses" checkbox and got the right value.

Mike, you are a lifesaver! I'll have to burn the code to see if that fixed it but it fixed the simulation issue.

The question I have is is this something that was done by Keil (since changing that checkbox fixed it) or is this something that was done/could have been avoided by me and if so, how? I'd like to be correct my programming practices if this is something I caused by missing an important programming step.

a) which bank is this ISR using
b) does this ISR call functions
c) is any such function called from Main as well
d) do you have interrupts of differnt priorities

and, finally, post the complete ISR you are running

Erik

The question I have is is this something that was done by Keil (since changing that checkbox fixed it) or is this something that was done/could have been avoided by me and if so, how?

A bit of both. But it's mostly your fault, for not paying enough attention to the documentation.

You called a function (the only one you've actually shown here) from another one that assumed a different register bank (your actual __interrrupt function). That is known and documented to cause incorrect program behaviour. So you shouldn't have done that.