Use hardware TWI for the PAT9125 (optical) filament sensor

[wavexx]

The PAT9125 on the MK3 is attached to the hardware SDA/SCL pins, but we're still using a software I2C implementation for some reason.

We add a new configuration option PAT9125_I2C (which is also enabled by default by this PR) to use the hardware TWI module in 400khz fast mode.

We save 170 bytes on the MK3S variant (which still requires probing to detect the current variant) and 268 on the MK3, plus a little bit of latency since despite the code still being synchronous, we can at least avoid having to wait for the stop condition.

PFW-1191

[vintagepc]

Three guesses what prompted this change 🤣
(and the first two don't count)

[wavexx]

On Wed, Sep 09 2020, vintagepc wrote: Three guesses what prompted this change 🤣

I couldn't sleep well after finding that out, had to fix it ;)

[vintagepc]

Did you end up testing it out by wiring up the hardware SPI sim? IIRC the SPIPeripheral class was written to support both bitbanged and "real" I2C connections and the fake PAT shouldn't care which it is wired to.

Edit: for anyone else wondering about the context for this, see
vintagepc/MK404#7 (comment)_

[wavexx]

Did you end up testing it out by wiring up the hardware SPI sim? IIRC the SPIPeripheral class was written to support both bitbanged and "real" I2C connections and the fake PAT shouldn't care which it is wired to.

Took a while, but getting back to this.
You mean the I2CPeripheral here?

At least without changes, the sensor doesn't seem to be detected right now. The SCL/SDA irqs do not seem to be generated in this case so the callbacks are never called.

[vintagepc]

Ah, ok. If you wouldn't mind tossing up a MK404 issue with a .hex and I'll take a look. It's possible that something is still unimplemented.

[vintagepc]

I don't have write access to the repo but I approve of this change regardless given its origins 😉

[DRracer]

@wavexx @vintagepc is this ready to get merged into MK3?

[vintagepc]

I think it is, yes :) All was working as expected in MK404

[DRracer]

OK, my only complaint is the usage of "digitalWrite()" - can we get rid of this?

[vintagepc]

This is what it's doing, seems like it should be possible to just strip out the unnecessary bits and just do the actions we need to set it high:

void digitalWrite(uint8_t pin, uint8_t val)
{
	uint8_t timer = digitalPinToTimer(pin);
	uint8_t bit = digitalPinToBitMask(pin);
	uint8_t port = digitalPinToPort(pin);
	volatile uint8_t *out;

	if (port == NOT_A_PIN) return;

	// If the pin that support PWM output, we need to turn it off
	// before doing a digital write.
	if (timer != NOT_ON_TIMER) turnOffPWM(timer);

	out = portOutputRegister(port);

	uint8_t oldSREG = SREG;
	cli();

	if (val == LOW) {
		*out &= ~bit;
	} else {
		*out |= bit;
	}

	SREG = oldSREG;
}

[leptun]

We can use the fastio macros here, can't we? Just fix the SCL and SDA definition in fastio so that it's fastio compatible

[wavexx]

We can remove it, but is it necessary? digitalWrite is already used elsewhere, so we won't save any space AFAIK. It's also not on a hot code path.

[wavexx]

Checking now if using FASTIO can lead to some space savings..

[wavexx]

Using fastio would actually save 12 bytes :), however I need to add some crummy defines just to make it build: including fastio.h is not enough, which is something that would be nice to fix.

#2690 would make sense IMHO

If including #2690 makes sense in this release, I could proceed with something less invasive (another copy of CRITICAL_SECTION.. uugh) to be revisited later @DRracer

[wavexx]

@DRracer now using fastio!