Conversation
gornekich
requested review from
DrZlo13,
bettse,
hedger and
skotopes
as code owners
| ADD_SCENE(picopass, emulate, Emulate) | ||
| ADD_SCENE(picopass, loclass, Loclass) | ||
| ADD_SCENE(picopass, key_input, KeyInput) | ||
| ADD_SCENE(picopass, dict_attack, DictAttack) |
There was a problem hiding this comment.
Some of these seem like changes that at independent of the NFC refactor. If that is the case, could they be pulled out and done as an independent PR(s) so that it is easier to see the changed the refactor requires?
There was a problem hiding this comment.
+1 on this. It seems like it's a NFC refactor, plus major cleanup at the same time.
I'm also concerned that some of the decisions, especially around emulation splitting to looping over an array of handlers rather than a switch statement, will affect emulation speed breaking compatibility with some readers. There's definitely some code that doesn't look the clanest but was done that way for performance reasons to make it work. Rather difficult determine what has broken something when there's so many things done together.
There was a problem hiding this comment.
@nvx sorry for late response, lost your message.
I think that introduced array of handlers in emulation part doesn't affect flipper response time much but gives cleaner code. Meanwhile we improved ISO15693 signal decoder and encoder in NFC library, which helped us to achieve better timings. Still we need to test it, and if it doesn't work good, we can apply some optimizations.
| SubmenuIndexWriteCustom, | ||
| }; | ||
|
|
||
| typedef struct { |
There was a problem hiding this comment.
This is an example of code I like, but feels unrelated and could probably go in beforehand (or after, which ever is easier)
|
|
I ran this through some testing Reading seems to be working as expected, tested cards with
I had some issues initially saving files, not sure if that was missing directories (fresh SD card) but it eventually started working. Emulation is working on SE (Rev E) and Signo readers. It is not working on Rev A or Rev C (multiclass non SE) while the older firmware is working. (I believe this has to do with iso15 vs iso14b @nvx would know better). When running loclass on a standard keyed reader it should indicate std keyed however it is crashing (with the vibration running) and needs a hard reset. Loclass was working as expected on an elite keyed reader. |
They both do 15693, but the RevA though C readers use shallow modulation/10% modulation index ASK, while the rest you tested that worked use OOK modulation. I haven't looked at the NFC refactor code yet, but if someone stopped using the IRQ pin and instead changed to MISO to read the incoming modulation from the card in the 15693 code that would explain the issue. Otherwise it could be a timing issue as well, each firmware seems to have slightly different thresholds for timing go figure. |
|
In an attempt to be helpful I created a PR that renamed RFAL_PICOPASS_BLOCK_LEN to make this PR smaller: #63 |
|
Want me to fix the conflicting files (since they are pretty much my fault)? |
No, thanks! |
|
Hello everyone, FW is almost ready: there will be one more tests iteration by QA team on Monday and then it will be released. |
|
As we agreed, I made another PR with smaller changes, which works with new NFC stack. Let's continue conversation there #68 |
6 participants
What's new
Merge after flipperdevices/flipperzero-firmware#3050 is merged
Verification
./fbt updater_packageufbt update -t f7 -l <local-sdk-path>ufbt launchChecklist (For Reviewer)