speeduino icon indicating copy to clipboard operation
speeduino copied to clipboard

Auxilliary input pin initialisation bug

Open Afroboltski opened this issue 4 years ago • 8 comments

There is a bug with the way Aux. input pins are initialised (and to be subsequently used for programmable I/O).

If an aux. input is assigned to pin x, the pinMode(pin, type) initialisation is done on pin x-1. This is not obvious because of the way the pin indices are defined in the INI file - DIGITAL_PIN is defined as strings starting from "1" as index 0, "2" as index 1, etc. hence why the offset is needed. But other defs use 0 as "Undefined" or "Disabled".

In the speeduino.ino file, we have

currentStatus.canin[currentStatus.current_caninchannel] = readAuxdigital((configPage9.Auxinpinb[currentStatus.current_caninchannel]&63)+1);

but this pin is initialised in sensors.ino,

byte pinNumber = (configPage9.Auxinpinb[currentStatus.current_caninchannel]&127); if( pinIsUsed(pinNumber) ) { BIT_SET(currentStatus.engineProtectStatus, PROTECT_IO_ERROR); } else { pinMode( pinNumber, INPUT); auxIsEnabled = true; }

causing a problem if a). pin 36 is already used (because we actually want pin 37, but the pinIsUsed check is done on pin 36), and b). this is the only aux. channel used, meaning that auxIsEnabled will never be set to true.

You may not even notice the problem if there were other aux inputs where x-1 didn't collide, and auxIsEnabled got set to true by "accident". Additionally the pinMode defaults to INPUT.

I believe byte pinNumber = (configPage9.Auxinpinb[currentStatus.current_caninchannel]&127); should be byte pinNumber = (configPage9.Auxinpinb[currentStatus.current_caninchannel]&63) + 1;

Afroboltski avatar Sep 10 '21 09:09 Afroboltski

Wow great catch! I was experiencing this problem first hand when I was setting up aux in 1 for an analog Input and i could never Get it to read. I ended up fiddling with it until it finally started showing values

shiznit304 avatar Sep 11 '21 23:09 shiznit304

https://github.com/noisymime/speeduino/pull/586#issuecomment-932878370

Corey-Harding avatar Oct 03 '21 07:10 Corey-Harding

Is there more involved in this than changing the two instance of byte pinNumber = (configPage9.Auxinpinb[currentStatus.current_caninchannel]&127); to byte pinNumber = (configPage9.Auxinpinb[currentStatus.current_caninchannel]&63) + 1; in sensors.ino?

I applied the fix above and PR #675 and still have issue #673

I was wondering if you had anymore insight on my issue.

Corey-Harding avatar Oct 03 '21 19:10 Corey-Harding

See my last comment on https://github.com/noisymime/speeduino/issues/673:

#654 was closed and "fixed" but whoever merged it only applied half my fix - Aux in1-16 should be Aux in 0-15 in the fullStatus defs at the top.

Afroboltski avatar Oct 03 '21 22:10 Afroboltski

See my last comment on #673:

#654 was closed and "fixed" but whoever merged it only applied half my fix - Aux in1-16 should be Aux in 0-15 in the fullStatus defs at the top.

See my update on #673 as well I think there is more to it than that

Corey-Harding avatar Oct 03 '21 23:10 Corey-Harding

Is this issue still present with current master release?

shiznit304 avatar Dec 13 '21 20:12 shiznit304

Yep, just had a look through the master branch (I haven't tested the code, just read through it, full disclosure).

Somehow all the fixes I have suggested have been undone, that is:

#define DIGITAL_PIN = "1", "2" ...

Means that

pinMode(DIGITAL_PIN "1") is actually doing pinMode(0) which is incorrect

and then in speeduino.ino

pseudo-code: readAuxdigital(DIGITAL_PIN "1" + 0) is actually readAuxdigital(0 + 1) which is correct.

(here's the full line: currentStatus.canin[currentStatus.current_caninchannel] = readAuxdigital((configPage9.Auxinpinb[currentStatus.current_caninchannel]&63)+1);)

Aaaaaaaan additionally:

#define fullStatus_def_2= "RPM DOT", ... "baro", "Aux in 1", "INVALID", "Aux in 2", "INVALID", ...

is STILL incorrect, it needs to start from "Aux in 0", "INVALID", "Aux in 1", ... "Aux in 15".

Cheers :)

Afroboltski avatar Dec 14 '21 08:12 Afroboltski

Last bit is on #676

Corey-Harding avatar Dec 14 '21 10:12 Corey-Harding