SPI: support for ISR and work_queue driven transfers

[dakejahl]

Added an _is_locked variable to the SPI class which is a simple bit mask where the bit position corresponds to the SPI bus number. When running SPI::transfer from an ISR, LockMode is selected as LOCK_NONE (because you cannot pend on a sem in interrupt context). I have added a check for the _is_locked flag for the given bus when LockMode is none, thus preventing an ISR driven SPI::transfer from clobbering an already active transfer taking place in workqueue/task/etc.

[dakejahl]

Next steps, test this with two IMUs (6500/9250) running on the same SPI bus (SPI1). Schedule one using the new px4_workq and the other with the current hrt_call_every. Look at spi_isr_deferred perf counter to see if the ISR driven transfer is being occasionally aborted.

[dakejahl]

Merged into #11261 and tested like so

	if(_whoami == MPU_WHOAMI_6500) {
		ScheduleOnInterval(_call_interval - MPU9250_TIMER_REDUCTION, 10000);
	} else {

		hrt_call_every(&_call,
			       1000,
			       _call_interval - MPU9250_TIMER_REDUCTION,
			       (hrt_callout)&MPU9250::measure_trampoline, this);
	}

This appears to be working correctly

And I am seeing perf counts for deferred ISR driven transfers

which match the bad transfers perf counter from a failed SPI::transfer

@dagar if you want to test it yourself
https://github.com/PX4/Firmware/tree/spi_locking_workq_and_isr

[davids5]

@dakejahl I am a tad bit uncomfortable with the removal of result, but I guess it can be added back if we extent the upstream nuttx SPI api.

Please verify the 2 comments raised

[dakejahl]

I removed result because _transfer was always returning OK. And yeah nuttx SPI exchange has no return value.

[dakejahl]

I need to do one more test on this Monday, I had left out applying the bus number to the bitmask when checking if the bus was locked. This shouldn't have had any affect with the way the system runs currently, I just want to sanity check it.

[dakejahl]

Okay I think this is done now. I fixed an issue where I wasn't checking the bit position in the _is_locked bit mask. So @dagar the ISR driven cycle is actually deferring much less than what was apparent previously (previously I was just looking at if anyone was using any SPI bus, i.e activity on SPI2).

This actually isn't relevant though because the default LOCK_MODE is LOCK_PREEMPTION anyways (enters critical section for transfer). However I changed it to LOCK_THREADS for the purpose of demonstrating this _is_locked mechanism is functional.

Bench tested on a Teal with 2 IMUs(6500/9250) on SPI1, one running from the new wq and the other from an hrt_call_every

[bkueng]

@dakejahl this needs to be atomic. It's a shared state potentially written to by multiple threads.
You can use the API in #11328.

Also this assumes that all SPI instances for a given bus run on the same thread. Can you add this as comment here to make the assumption explicit?

[dagar]

FYI once we're sure there's no longer a mix of PX4 SPI drivers running out of HRT and threads we can drop all of this and lean on the underlying nuttx SPI_LOCK (semaphore).

[dagar]

What I meant to emphasize was that this only exists for the case of a potential HRT and thread SPI conflict.

[dakejahl]

this needs to be atomic. It's a shared state potentially written to by multiple threads.
You can use the API in #11328.

Cool! I'll add that in as soon as #11328 is merged.

[dakejahl]

Also this assumes that all SPI instances for a given bus run on the same thread.

Could you explain please? I don't think it does.

[bkueng]

Could you explain please? I don't think it does.

The implementation is not correct if this does not hold. This is what can go wrong:

• 2 threads enter SPI::lock with the same bus id.
• both set _is_locked
• one of the threads (let's say thread1) enters the critical section, the other one (thread2) blocks
• thread1 leaves the CS and clears the bit in _is_locked.
• thread2 enters the CS, while the bit in _is_locked is cleared.

[dagar]

@dakejahl #11328 has merged.

[dakejahl]

@bkueng Thanks for the explanation! I've updated the code. Lock free atomic operations are a new concept to me, please let me know if this doesn't look right.

edit: Oh, ignore me. I need to check if the lock bit is already set before setting, otherwise it can potentially be "set" twice.

[davids5]

@dakejahl @dagar - Is it true that in the end (all done), there is only 1 thread per bus and the possibility of hrt interruption.?

If so:There is a 1:1 of bus:semaphore so this reduce to

interrupt context && dev->semaphore count == 0 -> exit

So the loser is always running in interrupt context. Therefore the test does not need protection as it is non interruptible - no nesting or HI priority IRQ) So dev->semaphore count == 0 => Exit. the sem_wait is already wrapped in a DI, EI and is safe.

Is the problem a shared bus? Barro and Nuttx driver on same bus?

[dagar]

I believe the only reason that wasn't done is accessing the somewhat hidden spidev semaphore.

[dakejahl]

Yeah I wasn't sure how to get at that semaphore. btw thanks everyone for reviewing, lots of little nuances I wasn't aware of with this.

[bkueng]

I need to check if the lock bit is already set before setting, otherwise it can potentially be "set" twice.

That's fine, if the at-most-one-thread-per-bus assumption holds (same as @davids5's question: 'Is it true that in the end (all done), there is only 1 thread per bus and the possibility of hrt interruption.?').

[davids5]

Given the " at-most-one-thread-per-bus assumption" has a complication we need to resolve to make it so.

A spi bus on some platforms can have a device that is accessed from a device driver in nuttx in the foreground. The params module can use a mtd derivative such as at25 eeprom or a ramtron FRAM and should have only non hrt threads running it. This needs to be true because the OS is used for the reader-writer serization (we should check with an test build with PX4_VERIFY_ASSERT(!up_interrupt_context()) in that driver)!

FMUv4 has on SPI2 a ms5611 barometer and the FRAM. The barometer data sheet states effectively do not wiggle SPI pins doing conversions. Hece this complication that I am not 100% solves the noise issues.

The only way this worked is because A) the ms5611 driver's bus transactions are on a work queue (ISR CAN NOT use OS functions that wait (sem_wait)) and B) it is not on a bus that has hrt threaded devices.

We need to catch a potential misuse: PX4_SPI_BUS_BARO == PX4_SPI_BUS_RAMTRON and some hrt device also on bus.

Which is the same problem the lock is needed for:

The default is locking_mode(LOCK_PREEMPTION), right-red path - this locks the hrt threads out.
the baro uses locking_mode(LOCK_THREADS)` the blue path which shares the dev->smaphore with the Nuttx Driver

1. hrt takes the left-red path - assume it owns the bus ONLY because of 2
2. The default non hrt (threads init or wq) take the right-red path - this locks the hrt threads out
3. baro (and the OS's drivers in spirit) takes right-blue path

The root problem is 1 can trounce on 3.

We should look at moving param module's IO to the SPIx bus thread (and enforce this architecturally) if we do that then everything will be on ONE bus thread and the we can then drop the sem_wait and use only the atomic for isr protection.

We should also change the parameter to not be written during flight. That will fully solve the barro noise issue. If we had adequate holdup time and a power fail detection we could commit them with an orderly shutdown as can be done with SW reboot. But that is a whole other issue.

[dakejahl]

Given the " at-most-one-thread-per-bus assumption" has a complication we need to resolve to make it so.

I think this makes sense architecturally and we should enforce it and document it.

We should also change the parameter to not be written during flight.

You are referring to _locking_mode? I agree.

That will fully solve the barro noise issue. If we had adequate holdup time and a power fail detection we could commit them with an orderly shutdown as can be done with SW reboot. But that is a whole other issue.

I think the way the ms5611 collects is:

1. Schedules the measurement on the hp_wq -> spi::transfer (tells ms5611 to measure)
2. Reschedules the collection phase on the hp_wq for some time in the future. Meanwhile ms5611 is performing a measurement, the SPI bus is not locked, and param module can use the bus.

I think we would need to keep the SPIx (spi2 on fmu-v4) bus locked for the entirety of the measure/collect of the ms5611 if the condition is "don't wiggle SPI while doing a conversion".

Yeah?

[davids5]

We should also change the parameter to not be written during flight.

You are referring to _locking_mode? I agree

No param save to the mtd.

I think the way the ms5611 collects is:

Schedules the measurement on the hp_wq -> spi::transfer (tells ms5611 to measure)
Reschedules the collection phase on the hp_wq for some time in the future. Meanwhile ms5611 is performing a measurement, the SPI bus is not locked, and param module can use the bus.

I think we would need to keep the SPIx (spi2 on fmu-v4) bus locked for the entirety of the measure/collect of the ms5611 if the condition is "don't wiggle SPI while doing a conversion".

I agree but only on HW where it matters and it can be done and that is case by case.

[dakejahl]

Okay, maybe we revisit this once #11261 is merged? I added the comment for the one thread per SPI bus assumption.

[dagar]

Another option to consider is to simply prevent the SPI transfer from an interrupt. Once #11571 is merged the only concern is 3rd party drivers rebased on newer PX4. I think it would be acceptable to fail at runtime with an error (or even assert) if it points to instructions that show how trivial it is to migrate.