Adds an LED_pulse feature using ISR to devices/grid_maze.py

[mathias-sm]

Hi everyone,

Thanks for maintaining this great ecosystem!

For an experiment we're running these days, I needed a feature to dim LEDs in a grid_maze setup. I'm doing this with PWM using a MicroPython timer and a callback that turns the LED on and off at the required duty_cycle (fraction of the time on), at the required freqency. The code is heavily inspired from

code/source/pyControl/hardware.py

Lines 420 to 450 in a54bdca

	def pulse(self, freq, duty_cycle=50, n_pulses=False, load_warning=True):
	# Turn on pulsed output with specified frequency and duty cycle.
	assert duty_cycle % 5 == 0, "duty cycle must be a multiple of 5 between 5 and 95%"
	if not self.timer:
	self.timer = pyb.Timer(available_timers.pop())
	self.fm = int(100 / next(x for x in (50, 25, 20, 10, 5) if duty_cycle % x == 0))
	self.off_ind = int(duty_cycle / 100 * self.fm)
	self.i = 0
	self.n_pulses = n_pulses
	if self.n_pulses:
	self.pulse_n = 0
	int_freq = freq * self.fm
	if load_warning and int_freq > 2000:
	warning("This pulse freq and duty_cycle will use > 10% of pyboard processor resources.")
	self.timer.init(freq=int_freq)
	self.timer.callback(self._ISR)
	self.on()
	@micropython.native
	def _ISR(self, t):
	self.i += 1
	if self.i == self.off_ind:
	if self.n_pulses:
	self.pulse_n += 1
	if self.pulse_n == self.n_pulses:
	self.off()
	return
	self.toggle()
	elif self.i == self.fm:
	self.i = 0
	self.toggle()

but as they are currently written, this maze's LED_on and LED_off are allocating new memory with to_byte

code/devices/grid_maze.py

Line 162 in a54bdca

self.I2C[i2c].mem_write(values.to_bytes(n_bytes, "little"), self.addr[exp_n], self.reg_addr[register], timeout=5)

which is incompatible with ISRs (see https://docs.micropython.org/en/latest/reference/isr_rules.html).

Instead, I pre-allocate buffers for on and off, and write those in a tight loop: this works, is as computationally light as I could think of, but makes pulsing incompatible with turning on/off other LEDs or delivering rewards. Feel free to merge, or otherwise leave here should anyone else need this feature! Happy to take the maintainers' input of course.

This is currently being tested in an experiment, so far we have not had any trouble but I will keep upstreaming fixes if need be. Thanks to @ThomasAkam for his input; some of this code was developed together with Arya Bhomick.

Cheers,
Mathias.

[mathias-sm]

Update: it turns out that the race condition between the pulse timer and other LED_on/LED_off/SOL_on/SOL_off can, in fact, sometimes occur. Let me fix this before merging.

I now think that the cleanest version of this would be to have a single buffer for the maze object, and when we want to communicate with I2C we read to the buffer, update it, and write from it. That way we only ever allocate it once, it can be used in the tight loop, and if the value gets updated outside of the pulse callback it's not going to mess up with the rest of the code.

[ThomasAkam]

Hi @mathias-sm,

Thanks for your work on this.

I now think that the cleanest version of this would be to have a single buffer for the maze object, and when we want to communicate with I2C we read to the buffer, update it, and write from it. That way we only ever allocate it once, it can be used in the tight loop, and if the value gets updated outside of the pulse callback it's not going to mess up with the rest of the code.

If I understand correctly you are proposing the following change to how setting the value of digital outputs works on the gridmaze hardware:

• In the current implementation, when the write_bit method is used to change the value of a single bit in a register, the state of the register is first read to a variable using read_register, then the bit is toggled in the variable, then the variable is written back to the register using write_register.
• You are proposing to maintain the current state of the GPIO registers that control the output pins as an attribute of the Gridmaze class, such that when writing a bit, instead of reading the current register state from the IC, the stored register value is just updated to set the bit and then written to the register.

I think this is a good plan because it will reduce the overheads associated with writing bits (as it removes the need for an I2C read) and will faciliate controlling outputs in interrupt callbacks. If you have time to have a go implementing this that would be great.