paralleldisplay: reset and read pins should be optional

[deshipu]

The reset and read pins should be optional, but the espressif
code had several places where it assumed they are not, and a bug that
caused a crash on release_displays if they were made optional.

The bug was caused by the fields for storing pin numbers being set
to NO_PIN, which has value of -1, while the fields have type
uint8_t. That set the actual value to 255, and a subsequent
comparison to NO_PIN returned false.

[dhalbert]

In other ports, NO_PIN is 0xff. In this port, it is set by #define NO_PIN (GPIO_NUM_NC), where GPIO_NUM_NC is -1, but if NO_PIN were changed to 0xff, it would make changing things to int8_t not necessary. I also think it would be better not to use a signed byte for a pin number, in case we have pins > 128.

[deshipu]

You can always change it to int16_t when needed, it's not like we are saving loads of memory here. And by the same argument, what if we have 255 pins?

[dhalbert]

I am just saying I think the initial idea of using #define NO_PIN (GPIO_NUM_NC) was not the best choice.

[deshipu]

Isn't it needed for compatibility with idf?

[dhalbert]

Thanks for the change! I'd like @tannewt to comment on the overall PR: I don't have much experience with displays.

[gamblor21]

Just a quick late night look but I do not think the atmel or rp2040 ports will work if the read pin is left out (likely a bug as well with them). Also in the espressif port I noticed common_hal_never_reset_pin(read); was removed but never replaced if read was specified which I believe means if it is given it would be reset when the other display pins would not be.

[deshipu]

Just a quick late night look but I do not think the atmel or rp2040 ports will work if the read pin is left out (likely a bug as well with them).

You are right! I started this with the reset pin, and after checking in all the ports that they do have the code to make it optional, I noticed that the espressif port also has that code for the read pin, and assumed the other ports would as well. I will fix that.

Also in the espressif port I noticed common_hal_never_reset_pin(read); was removed but never replaced if read was specified which I believe means if it is given it would be reset when the other display pins would not be.

It was there twice: https://github.com/adafruit/circuitpython/pull/6118/files#diff-6e4f17b3d8ddfa1af71ef1d81695db73526478965d2f456c14574a98bd32bb47R81

[deshipu]

Looks like making the read pin optional took the build size over the edge for the Japanese translation of Matrix Portal M4. Should I revert that and just leave the reset pin as optional, or can we do something to make that one build fit?

[tannewt]

The matrixportal build has paralleldisplay enabled and clearly doesn't need it. You can disable it and then the build should fit.

[tannewt]

This all looks good to me.

[dhalbert]

I shrank the matrixportal build here: eff6057

So if that works, and gets merged, we can merge this afterwards and you won't have to submit that patch.

[dhalbert]

Can merge now with shrink in from #6117.