marcrdavis / ArduinoTITO-PlayerTracking

A homebrew slot machine TITO, player tracking and display project
28 stars 12 forks source link

Are you building the currently uploaded code? #3

Closed jtwine closed 3 years ago

jtwine commented 3 years ago

Both versions of the code (magstripe or RFID) appear to have bugs that will prevent building out of the box, I thought I was looking at a library/version issue, but there is code that just contains errors. The waitForResponse function is a good example:

byte waitForResponse(byte & waitfor, byte * ret, int sz)
{
  byte responseBytes[sz - 2];
  int wait = 0;

  while (Serial1.read() != waitfor && wait < 3000) {
    delay(1);
    wait += 1;
  }

  if (wait >= 3000) {
    Serial.println(F("Unable to read data - timeout"));
    memset(ret, 0, sz);
    sasError = true;
    return ret;
  }

  Serial1.readBytes(responseBytes, sizeof(responseBytes));
  ret[0] = {0x01};
  ret[1] = waitfor;
  memcpy(ret + 2, responseBytes, sizeof(responseBytes));

  return ret;
}

The parameter sz is not constant and cannot be used to stack-allocate the responseBytes array. The ret parameter is of type byte* but the code is trying to return it instead of a byte. (That return value is not being used, anyway.)

There are other similar methods that look more like pesudocode than working C/C++. In setupPlayerMessage this if statement is present: if (playerComps>1 and !skipBuffer) - note the and instead of &&.

Was there some post-download or pre-compile step that I need to perform that I missed? Or was this just some accidental upload?

Thanks!

Peace!

-=- James.

marcrdavis commented 3 years ago

Hi James – Hmm – you might be right about the setupPlayerMessage code – but it is compiling and working on my side – very odd. I will fix that.

The waitForResponse code is working fine though and the code does compile.

You will get compile warnings (if you have them enabled) but the code will compile fine. I have had multiple folks testing both versions for a few weeks now on different games and everything has been fine.

Marc

From: jtwine @. Sent: Wednesday, March 24, 2021 1:47 PM To: marcrdavis/ArduinoTITO-PlayerTracking @.> Cc: Subscribed @.***> Subject: [marcrdavis/ArduinoTITO-PlayerTracking] Are you building the currently uploaded code? (#3)

Both versions of the code (magstripe or RFID) appear to have bugs that will prevent building out of the box, I thought I was looking at a library/version issue, but there is code that just contains errors. The waitForResponse function is a good example:

byte waitForResponse(byte & waitfor, byte * ret, int sz) { byte responseBytes[sz - 2]; int wait = 0;

while (Serial1.read() != waitfor && wait < 3000) { delay(1); wait += 1; }

if (wait >= 3000) { Serial.println(F("Unable to read data - timeout")); memset(ret, 0, sz); sasError = true; return ret; }

Serial1.readBytes(responseBytes, sizeof(responseBytes)); ret[0] = {0x01}; ret[1] = waitfor; memcpy(ret + 2, responseBytes, sizeof(responseBytes));

return ret; }

The parameter sz is not constant and cannot be used to stack-allocate the responseBytes array. The ret parameter is of type byte* but the code is trying to return it instead of a byte. (That return value is not being used, anyway.)

There are other similar methods that look more like pesudocode than working C/C++. In setupPlayerMessage this if statement is present: if (playerComps>1 and !skipBuffer) - note the and instead of &&.

Was there some post-download or pre-compile step that I need to perform that I missed? Or was this just some accidental upload?

Thanks!

Peace!

-=- James.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/marcrdavis/ArduinoTITO-PlayerTracking/issues/3 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ARPXAALCLCWGALKT6DQQASLTFIQRJANCNFSM4ZXX27BA . https://github.com/notifications/beacon/ARPXAAMZZKK4SH4RWFUWVTTTFIQRJA5CNFSM4ZXX27BKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4MQRIDDQ.gif

marcrdavis commented 3 years ago

Regarding the waitForResponse code; sz is not a constant; the responseBytes array is sized differently depending on the size of the input array, less two bytes.

Yes - the return is not being used; It could probably be rewritten more elegantly. That function went through several revisions and I may have just left the declaration unmodified.

jtwine commented 3 years ago

Can you tell me which environment/toolchain you are using to build this? Whichever one is compiling the code as-is correctly? (Unless I missed it in the Wiki/Readme/Docs?)

Thanks!

Peace!

-=- James.

marcrdavis commented 3 years ago

I’m using the Arduino IDE version 1.8.13;

Be sure to update your \Libraries folder with the modified libraries included in the zip file.

From: jtwine @. Sent: Thursday, March 25, 2021 12:22 PM To: marcrdavis/ArduinoTITO-PlayerTracking @.> Cc: marcrdavis @.>; Comment @.> Subject: Re: [marcrdavis/ArduinoTITO-PlayerTracking] Are you building the currently uploaded code? (#3)

Can you tell me which environment/toolchain you are using to build this? Whichever one is compiling the code as-is correctly? (Unless I missed it in the Wiki/Readme/Docs?)

Thanks!

Peace!

-=- James.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/marcrdavis/ArduinoTITO-PlayerTracking/issues/3#issuecomment-807037843 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ARPXAAIG4SXGPA4H4FWVPGLTFNPJHANCNFSM4ZXX27BA . https://github.com/notifications/beacon/ARPXAAIV4GUFXF7EBYKPUQLTFNPJHA5CNFSM4ZXX27BKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGANGXEY.gif

jtwine commented 3 years ago

OK - I see what my problem is. A non-constant array size (VLA) is an optional feature of C11 that is supported by GCC but not all compilers. That is why that code was tripping me up. I would have implemented the same kind of behavior using alloca(...), which is not available on all platforms, but this is essentially the same thing.

This, BTW, is the same reason and works in the if expression - and is an alternative token for && that is supported by some C++ compilers (like GCC).

Thanks and sorry for my confusion.

Peace!

-=- James.

marcrdavis commented 3 years ago

I’m going to cleanup the warnings in the next release. Thx

Marc

From: jtwine @. Sent: Thursday, March 25, 2021 1:17 PM To: marcrdavis/ArduinoTITO-PlayerTracking @.> Cc: marcrdavis @.>; Assign @.> Subject: Re: [marcrdavis/ArduinoTITO-PlayerTracking] Are you building the currently uploaded code? (#3)

OK - I see what my problem is. A non-constant array size (VLA) is an optional feature of C11 that is supported by GCC but not all compilers. That is why that code was tripping me up. I would have implemented the same kind of behavior using alloca(...), which is not available on all platforms, but this is essentially the same thing.

This, BTW, is the same reason and works in the if expression - and is an alternative token for && that is supported by some C++ compilers (like GCC).

Thanks and sorry for my confusion.

Peace!

-=- James.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/marcrdavis/ArduinoTITO-PlayerTracking/issues/3#issuecomment-807124634 , or unsubscribe https://github.com/notifications/unsubscribe-auth/ARPXAAO6CZTYQ3G5X7FXBWLTFNVX5ANCNFSM4ZXX27BA . https://github.com/notifications/beacon/ARPXAAL3UZWJUT52XTGTKT3TFNVX5A5CNFSM4ZXX27BKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOGAN35GQ.gif