-
Notifications
You must be signed in to change notification settings - Fork 606
Implementation common operations for ISO15693 With PN5180 #2381
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.
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
Conversation
@dotnet-policy-service agree |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Ellerbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this nice addition. Couple of comments and questions.
Can you also please adjust the main readmes to add the support for the new cards?
Thanks!
| ser[10] = BlockNumber; | ||
| ser[11] = (byte)(Data.Length / 4); | ||
| Uid?.CopyTo(ser, 2); | ||
| Data.CopyTo(ser, 12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused on this code - i.e. in RunIcodeCardCommand you set Data after you run this method - what will be copied in this case? (since I don't know the internals of protocol this might be expected but it would be nice if you could add a comment in that case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I've mentionned in the comment, Data means the data which has been read or to write for the specific block. The data which want to write will copied to request data in Serialize(), after run the method RunIcodeCardCommand , assigns response data to Data. Will add comments in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine with me if consistent with existing code - will let @Ellerbach review and decide since he's main owner of this piece of code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern indeed used for the other cards.
krwq
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall - nice job on getting this working - treat my comments as optional
Ellerbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now. Thanks a lot for the adjustments!
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hey @czwy I think something went wrong with merges here, I'd recommend to locally |
Ellerbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great now. Thanks. A minimal remark for performances reasons.
src/devices/Card/Icode/IcodeCard.cs
Outdated
| byte[] dataOut = new byte[_responseSize]; | ||
|
|
||
| var ret = _rfid.Transceive(Target, requestData, dataOut.AsSpan(), NfcProtocol.Iso15693); | ||
| _logger.LogDebug($"{nameof(RunIcodeCardCommand)}: {_command}, Target: {Target}, Data: {BitConverter.ToString(requestData)}, Success: {ret}, Dataout: {BitConverter.ToString(dataOut)}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's minimal but it can have some performances impact. The $"wih everything inside" expression is always calculated then the LogDebug function is called and inside, there is a check about the log level saying that the log should be displayed or not.
Point here is that the creation of the expression, debug or not is always processed.
So, can you please check the log level and then call the LogDebug is we're indeed in debug mode?
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Ellerbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good to me. Nice addition!
Implementation common operations for ISO15693 With PN5180,including:Listening ISO15693 card(Inventory), read sinlge block, write single block, read multiple blocks, stayquiet, lockblock, reset to read, select, LockAFI, LockDSFID, GetSystemInformation.
Microsoft Reviewers: Open in CodeFlow