ST25TB poller refining + write support

[augustozanellato]

What's new

• reworked async poller
• added sync poller
• added write support

This PR should be ~same to #3185 but based on the improved NFC APIs

Verification

• Read a ST25TB and verify it's still read correctly

Checklist (For Reviewer)

• PR has description of feature/bug or link to Confluence/Jira task
• Description contains actions to verify feature/bugfix
• I've built this code, uploaded it to the device and verified feature/bugfix

[gornekich]

Hello @augustozanellato , thanks for PR!
First of all - everything works fine. However there are some things I want to discuss.

It would be nice if all pollers in NFC library looks similarly. That would help to maintain NFC subsystem a lot. I left some suggestions in code review.

Also I want to discuss poller states. Since your poller becomes bigger and you already added write capabilities in sync API, I suggest expanding poller itself. I would make these states in poller:

• Idle - reset St25tbPoller instance
• Select - selects card, emit "Ready" event with card type in event data
• Request mode - emit "RequestMode" event and receive mode in event data. Modes are St25tbPollerModeRead and St25tbPollerModeWrite
• Read - reads tag and switches to Fail or Success states
• Write - emits "RequestBlockData" event and receive block and block number in event data. Performs write command and switches to Fail or Success states.
• Fail - emits "Fail" event with error code in event data. Ends with furi_delay_ms(100) to switch context and switches to Idle state.
• Success - emits "Success" event with error code in event data. Ends with furi_delay_ms(100) to switch context and switches to Idle state.

In this case your sync API would be easy to write and we will have easier way to implement "write" in NFC application. You can see how it's done in mf classic poller for example.

Please, let me know what you think about this. We can do this step by step: merge this PR first and then refactore st25tb poller in future

[gornekich]

Also please don't forget to lint sources :)
./fbt format

[augustozanellato]

Also please don't forget to lint sources :) ./fbt format

I usually have format on save enabled, I'll figure out why it didn't work this time.

It would be nice if all pollers in NFC library looks similarly.

The thing that made me architect this poller different from the other ones was that this one is kinda an exception from the standard poller in the sense that it doesn't have either a parent or any children protocols, but has to implement both "low level" (anticollision, selection etc) and "high level" (data reading and writing).

[augustozanellato]

Please, let me know what you think about this. We can do this step by step: merge this PR first and then refactore st25tb poller in future

I think we should just iterate on this pr until we get to a decent shape without breaking API even more times :)

[augustozanellato]

It took less time than expected.
There are two deviations from your suggestions:

• there isn't an "idle" state, i think it'd be kinda useless, feel free to correct me on that;
• there isn't a RequestBlockData event because I made RequestMode handle mode arguments.

[gornekich]

Well done, thank you! I think poller looks much better now

[augustozanellato]

Well done, thank you! I think poller look much better now

Thanks for your suggestions :)

[gornekich]

there isn't an "idle" state, i think it'd be kinda useless, feel free to correct me on that;

No problems with that. Usually I add Idle state to set all variables in poller instance to default values. Since there are not so many variables in this poller, Idle state is useless. If the poller will grow, we can add this state if necessary.

there isn't a RequestBlockData event because I made RequestMode handle mode arguments.

I was thinking about different Write mode behavior - to call "RequestBlockData" event, write blocks until upper layer provides data. When upper layer stops providing data poller switches to Success state. This is quite flexible behavior which allows you to write not only one block after anticollision, but perform multiple write operations in one communication session.
I don't see any reasons to change it right. We can return to this later, if we want to add "Write" option in NFC application. These changes will cause minor API update.

[augustozanellato]

there isn't an "idle" state, i think it'd be kinda useless, feel free to correct me on that;

No problems with that. Usually I add Idle state to set all variables in poller instance to default values. Since there are not so many variables in this poller, Idle state is useless. If the poller will grow, we can add this state if necessary.

Yeah that makes sense.

there isn't a RequestBlockData event because I made RequestMode handle mode arguments.

I was thinking about different Write mode behavior - to call "RequestBlockData" event, write blocks until upper layer provides data. When upper layer stops providing data poller switches to Success state. This is quite flexible behavior which allows you to write not only one block after anticollision, but perform multiple write operations in one communication session. I don't see any reasons to change it right. We can return to this later, if we want to add "Write" option in NFC application. These changes will cause minor API update.

I didn't think about that, but that could be solved by having st25tb_poller_success_handler set poller state to St25tbPollerStateRequestMode so that in a communication session an arbitrary number of operations can be done. Tell me what do you think about this paradigm.

[gornekich]

I didn't think about that, but that could be solved by having st25tb_poller_success_handler set poller state to St25tbPollerStateRequestMode so that in a communication session an arbitrary number of operations can be done. Tell me what do you think about this paradigm.

Yes, that's actually a good solution. Will you do that in this PR?

[augustozanellato]

Yes, that's actually a good solution. Will you do that in this PR?

Done, I was just waiting for some feedback :)

[gornekich]

Perfect, thanks!

[augustozanellato]

The failing unit test is subghz related, I don't think that could be directly related to this pr 🤔

[gornekich]

Yes, the problem is not this PR

[augustozanellato]

Looks like extra - read specific card type - st25tb is now broken, I'll hook up a debugger and figure out why.

[augustozanellato]

Bug fixed, while investigating that one I also found another one, more details in commit message.
PR should now be ready to review and merge.