SPI read from normal code and ISR code advice (AD7902)

[allenhuffman]

With the PCD PIC24 compiler, if you have any function that is called from an ISR, and also used outside of the ISR, the compiler generates warnings and will mask interrupts around this function. That is an interesting feature that means the developer does not have to do that work. (This makes me uncomfortable.)

I am looking into some of our code where we read a SPI part (AD7902). It connects to an RF detector, and the RF signal can be CW (constant) or PWM (pulsing).

This board generates the RF signal, and then uses the PIC24 pwm_on() capability to do the pulsing. This allows the code to use an ISR to catch when the pulse happens, and read the RF detector during the pulses.

The architect of this code created duplicate functions - such as:

[Ttelmah]

The duplication is the way to avoid this. However using this on the SPI
hardware, worries me. Problem is that since this is hardware, it is a single
resource. If the 'main' code is running and operates the SPI, and while
this is happening the ISR triggers and does another operation on the
SPI, the flags on the return will not be those from the main operation, and
the byte sent in the ISR will come between two being sent in the master.
This would be a problem. I suspect this does not happen, because the 'main'
operations on the SPI, stop while the ISR is running?. If this is the case, then
really the problem need never occur. I think you need to look carefully at
when the transactions actually occur. If the two operations are actually
separated like this, then just be explicit and change where the interrupts
are enabled so the problem disappears.

[allenhuffman]

Thanks --- for this application, we are either generating a pulsed PWM signal, or a CW constant signal, so only one is used at a time.

CCS has confirmed that the I/O is not interrupts (already protected) but user code that is doing multiple SPI or I2C calls in series could obviously be interrupted.

If duplicating code is the way it is done here, then so be it. Less rework for me

I am going to make in inquiry with CCS to see if there is a #pragma or similar override that will prevent the IRQ masking around functions, which would allow us to do it manually when we know it needs to be done.
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?

Using: 24FJ256GA106, 24EP256GP202, 24FJ64GA002 and 24FJ1024GA606.

[Ttelmah]

I wonder if the NESTED_INTERRUPTS setup might accidentally do this?.
Might just be worth trying?.

[allenhuffman]

[Ttelmah]

Yes, as I said 'between bytes'.
It would depend totally on the nature of the packets of data, what triggers
things and the whole relationship between the internal code and the
external.
Is it possible you could avoid the whole problem by doing all the SPI
inside the IRQ?. When the main code wants to do something to the
device, it sets a flag, and the SPI code in the IRQ, if it sees this flag,
does the extra SPI after it's timed stuff. This way the extra SPI only
happens in the gaps in the PWM triggered SPI.
Would get rid of the need to have the two streams, and the need for
code duplication.

[allenhuffman]

[newguy]

This topic has stirred memories....

Place I started with greeted me on my first day with their suite of products along with "they work." That was determined to likely be a lie minutes later when a technician told me that the system .... well let's just say he disagreed that it worked.

Some months later I had to delve into the FW of a particular board that, again, "worked" but with a litany of weird things that sometimes happened. I found that the code was structured to access an SPI memory both inside of an ISR and outside of it in the main loop. With no protections to prevent the ISR from trampling the main code's access either.

Sorry Allen - in that particular situation it made the most sense to remove the ISR's SPI code entirely. That said, would it be possible to set a flag, SPI_IN_USE_FLAG = TRUE in your main loop code (the code outside of the ISR)? Your ISR can test the condition of this flag and either wait for the peripheral to complete the transaction it currently has in progress or cause the premature cessation of the transaction. All SPI ICs I've worked with reliably and repeatably reset their internal workings as soon as the /CS is brought high. You can also have the ISR set another flag - SPI_TRANSACTION_RESET_FLAG which your main code can use to deduce whether to repeat the transaction fresh once the ISR has done its duty.

[Ttelmah]

I think it is critical to only do the transactions in one location.
As Newguy says outside the ISR would be the easiest, but the
alternative would be to do everything in the ISR. Since there is a need
for the SPI transaction on the signal edge to be accurately timed, it
makes doing these in the ISR probably best.
So, instead of just a single value flag, have a flag with multiple values.
Perhaps INITIALISE, EDGE, and any other transactions that need to
happen. Then have the ISR setup, so if it is called with 'INITIALISE;,
it performs the device initialisation, while if it is called with EDGE set,
it does the handling for the PWM edge. Add other values for whatever
other transactions are needed. Then in the main code, when you want to
start the device, set the flag to INITIALISE, and set the interrupt flag.
The ISR seeing this flag is set then performs the initialisation. Have
the ISR default to setting the flag to EDGE on it's exit, so the current
interrupt code becomes the default behaviour.
This way all the transactions become granular, done in the single
location of inside the ISR.
Ideally the calling code would be:
disable interrupt
set flag to required value
set interrupt flag
enable interrupt. Handler is called.

This means there is no possibility of the edge transaction being called
at the moment the flag is set.

The ISR, can just test the flag as it's first operation, and only do the
extra transactions if it is not 'EDGE'.

[allenhuffman]

Thanks to both of you. It does make me think I am on the correct path.

This morning I am going to try doing IRQ masking around the other SPI calls in this firmware and see if that changes behavior. Nasty, but a good way to see what is going on.

I have noticed we get crashes when we change frequency, and that is a SPI message inside driver code for an ADF4351 RF synthesizer chip

We also use SPI when we are adjusting the attenuators that control power output, which happens constantly when running as the system maintains the target power.

The game is afoot!
_________________
Allen C. Huffman, Sub-Etha Software (est. 1990) http://www.subethasoftware.com
Embedded C, Arduino, MSP430, ESP8266/32, and PIC24 programmer.
http://www.whywouldyouwanttodothat.com ?

Using: 24FJ256GA106, 24EP256GP202, 24FJ64GA002 and 24FJ1024GA606.

[allenhuffman]

[Ttelmah]

Yes, claim/release of the device makes sense. That should avoid the
issues. That it seems to have fixed the problem suggests this is what
was causing the problem.

[allenhuffman]