Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

linked responds bugfix #331

Closed
wants to merge 2 commits into from
Closed

linked responds bugfix #331

wants to merge 2 commits into from

Conversation

Sleepychord
Copy link

fix the bug when receiving linked responds

@Sleepychord
Copy link
Author

Maybe there is something wrong in Travis CI......? It mistook variables' scope!

Rotzbua
Rotzbua requested changes Jul 27, 2017
View reviewed changes
src/MFRC522Extended.cpp Outdated

result = TCL_TransceiveRBlock(tag, true, ackData, &ackDataSize);
byte linked = 0;
result = TCL_TransceiveRBlock(tag, true, ackData, &ackDataSize, linked);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linked should be a pointer

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very sorry to make such a silly mistake and I think I have fixed it.

src/MFRC522Extended.h Outdated
@@ -93,7 +93,7 @@ class MFRC522Extended : public MFRC522 {
/////////////////////////////////////////////////////////////////////////////////////
StatusCode TCL_Transceive(PcbBlock *send, PcbBlock *back);
StatusCode TCL_Transceive(TagInfo * tag, byte *sendData, byte sendLen, byte *backData = NULL, byte *backLen = NULL);
StatusCode TCL_TransceiveRBlock(TagInfo *tag, bool ack, byte *backData = NULL, byte *backLen = NULL);
StatusCode TCL_TransceiveRBlock(TagInfo *tag, bool ack, byte *backData = NULL, byte *backLen = NULL, byte* linked = NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use c++11 nullptr instead of NULL

src/MFRC522Extended.cpp
// Initialize the receiving data
// TODO Warning: Value escapes the local scope
in.inf.data = outBuffer;
in.inf.size = outBufferSize;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep the space

@Rotzbua Rotzbua mentioned this pull request Jul 27, 2017
Open
Rotzbua
Rotzbua reviewed Jul 27, 2017
View reviewed changes
src/MFRC522Extended.cpp
// Result is chained
// Send an ACK to receive more data
// TODO: Should be checked I've never needed to send an ACK

while (in.prologue.pcb & 0x10) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TCL_TransceiveRBlock the var linked is set:

*linked = in.prologue.pcb & 0x10;

and then at the end of the while it is checked

if(!linked) break;

But that's the same as

while (in.prologue.pcb & 0x10)

Or did I missed something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, the variables "in"s are local variables. So while (in.prologue.pcb & 0x10) in function TCL_Transceive will always be true although in.prologue.pcb in function TCL_TransceiveRBlock will change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. You are right.

@Rotzbua Rotzbua added the need_review ↩️ awaiting review from people (before merge) label Jul 28, 2017
@Rotzbua Rotzbua added this to the 1.4.x milestone Jul 28, 2017
@Rotzbua
Copy link
Collaborator

Rotzbua commented Jul 28, 2017

In 2-3 weeks I will have a closer look on it. Thanks so far for support :)

Rotzbua
Rotzbua requested changes Jan 11, 2018
View reviewed changes
Copy link
Collaborator

@Rotzbua Rotzbua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay. I do not have the time to get deeper in this extended class.

I suggest changing linked to an boolean.
Please remove the not required space changes, it makes changes unnecessarily huge. Thanks.

src/MFRC522Extended.cpp
while (in.prologue.pcb & 0x10) {
byte ackData[FIFO_SIZE];
byte ackDataSize = FIFO_SIZE;

result = TCL_TransceiveRBlock(tag, true, ackData, &ackDataSize);
byte linked = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linked seems more a boolean and should be

src/MFRC522Extended.cpp
// Result is chained
// Send an ACK to receive more data
// TODO: Should be checked I've never needed to send an ACK

while (in.prologue.pcb & 0x10) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. You are right.

src/MFRC522Extended.cpp
@@ -859,7 +856,7 @@ MFRC522::StatusCode MFRC522Extended::TCL_Transceive(TagInfo *tag, byte *sendData
/**
* Send R-Block to the PICC.
*/
MFRC522::StatusCode MFRC522Extended::TCL_TransceiveRBlock(TagInfo *tag, bool ack, byte *backData, byte *backLen)
MFRC522::StatusCode MFRC522Extended::TCL_TransceiveRBlock(TagInfo *tag, bool ack, byte *backData, byte *backLen, byte* linked)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool* linked

src/MFRC522Extended.cpp
@@ -913,7 +911,8 @@ MFRC522::StatusCode MFRC522Extended::TCL_TransceiveRBlock(TagInfo *tag, bool ack
*backLen = in.inf.size;
memcpy(backData, in.inf.data, in.inf.size);
}

if(linked)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest for bool:

if(linked) {
     *linked = ((in.prologue.pcb & 0x10)!=0x00);
}

src/MFRC522Extended.h
@@ -93,7 +93,7 @@ class MFRC522Extended : public MFRC522 {
/////////////////////////////////////////////////////////////////////////////////////
StatusCode TCL_Transceive(PcbBlock *send, PcbBlock *back);
StatusCode TCL_Transceive(TagInfo * tag, byte *sendData, byte sendLen, byte *backData = NULL, byte *backLen = NULL);
StatusCode TCL_TransceiveRBlock(TagInfo *tag, bool ack, byte *backData = NULL, byte *backLen = NULL);
StatusCode TCL_TransceiveRBlock(TagInfo *tag, bool ack, byte *backData = NULL, byte *backLen = NULL, byte* linked = nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool* linked

src/MFRC522Extended.cpp
@@ -701,13 +702,11 @@ MFRC522::StatusCode MFRC522Extended::TCL_Transceive(PcbBlock *send, PcbBlock *ba

outBufferOffset += 2;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep the space

src/MFRC522Extended.cpp
@@ -807,17 +806,14 @@ MFRC522::StatusCode MFRC522Extended::TCL_Transceive(TagInfo *tag, byte *sendData
out.inf.size = 0;
out.inf.data = NULL;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep the space

src/MFRC522Extended.cpp
// Check chaining
if ((in.prologue.pcb & 0x10) == 0x00)
return result;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep the space

src/MFRC522Extended.cpp
@@ -828,19 +824,19 @@ MFRC522::StatusCode MFRC522Extended::TCL_Transceive(TagInfo *tag, byte *sendData
*backLen = in.inf.size;
memcpy(backData, in.inf.data, in.inf.size);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep the space

src/MFRC522Extended.cpp
result = TCL_Transceive(&out, &in);
if (result != STATUS_OK) {
return result;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep the space

@Rotzbua Rotzbua added in_progress ⏳ and removed need_review ↩️ awaiting review from people (before merge) labels Jan 11, 2018
@Rotzbua Rotzbua added abandoned_can_be_adopt ⏲️ pull request is abandoned, can be adopt by other user and removed in_progress ⏳ labels Feb 25, 2018
@Rotzbua Rotzbua removed this from the 1.4.x milestone Feb 25, 2018
@Rotzbua Rotzbua closed this Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rotzbua Rotzbua Rotzbua requested changes

Assignees
No one assigned
Labels
abandoned_can_be_adopt ⏲️ pull request is abandoned, can be adopt by other user
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Sleepychord @Rotzbua