RFC: drivers: rfid: add CR95HF driver with SPI support

[cmm1981]

This pull request introduces a new RFID API and a driver for the STMicroelectronics CR95HF RFID chip.

🔧 Current capabilities:

• SPI interface only (UART not yet implemented)
• Basic RFID API definition
• CR95HF initialization
• Switching to Tag Detection mode
• Reading the UID of detected tags

🚧 Limitations:

• UART mode not supported yet
• Currently supports only ISO14443A tag detection

Signed-off-by: Conny Marco Menebröcker c-m-m@gmx.de

[faxe1008]

Not really against using tools what so ever, but this seems to be heavily LLM generated, with very obvious blunders. 😉

If I recall correctly an RFID API was discussed several times in the past (I think @pdgendt was involved at some point). Maybe it would make sense to first check the requirements of the API, discuss it's design and then dive into implementing a sample driver.

Besides that its also missing a build_all test of sorts, none of these things are currently tested by CI.

[alexanderwachter]

I'm missing an API to execute the activation sequence and collision loop.

Some method like this in the driver API:
rfid_select_mode(RFID_FIELD_ON);
rfid_command(RFID_REQA);
....

And something in this direction in a subsys API:

struct picc_context;
const struct device dev* = DT_DEVICE_GET(reader);
rfid_int(dev, &picc_context);

while(true) {
int ret = rfid_detect(&picc_context);
// somehow check if correct card and exit
}

[cmm1981]

Not really against using tools what so ever, but this seems to be heavily LLM generated, with very obvious blunders. 😉

That hurts...LLM was only used for the comments and for analyzing the ISO 14443...
But I don't have much Zephyr experience. It is my first project. And I am doing it in my spare time.
I think that is the reason for the blunders.

If I recall correctly an RFID API was discussed several times in the past (I think @pdgendt was involved at some point). Maybe it would make sense to first check the requirements of the API, discuss it's design and then dive into implementing a sample driver.

I am open for discussions. Currently I focused on what I need, which is only reading the UID.
I don't have much experience with RFID; so far, I only know the datasheet of the CR95HF and what LLMs have told me about ISO14443. Unfortunately, I don't have access to the ISO standard itself. That's why I'm glad if people who are more deeply involved in the subject help define the API

Besides that its also missing a build_all test of sorts, none of these things are currently tested by CI.

I first wanted to see how my proposal would be received before putting more work into it. If the driver and API have a chance, I’ll also work on tests and additional features.

[cmm1981]

I'm missing an API to execute the activation sequence and collision loop.

Some method like this in the driver API: rfid_select_mode(RFID_FIELD_ON); rfid_command(RFID_REQA); ....

For the CR95HF there is not 'Field on' command. The field is switched on if the user selects a protocol.
If it is common for other chips, maybe I can implement in the way, that a default protocol is activated.

And something in this direction in a subsys API:

struct picc_context; const struct device dev* = DT_DEVICE_GET(reader); rfid_int(dev, &picc_context);

while(true) { int ret = rfid_detect(&picc_context); // somehow check if correct card and exit }

In my application I have currently the following in a loop:

rfid_select_mode(rfid_dev, TAG_DETECTOR);

rfid_protocol_select(rfid_dev, ISO_14443A);

rfid_get_uid(rfid_dev, uid, &uid_len);

There I can check if the uid is correct. But sure, I could write a detect function which is doing this.

[cmm1981]

I actually expected more discussion regarding the API. So far, only @alexanderwachter has provided feedback. The features he requested are, in principle, already included in my API, but implemented differently than he expected. Below is a brief comparison:

Function (Alexander)	Equivalent in My API	Remarks
rfid_select_mode(RFID_FIELD_ON)	rfid_select_mode(rfid_dev, TAG_DETECTOR) rfid_protocol_select(rfid_dev, ISO_14443A)	Requires two separate calls
rfid_command(RFID_REQA)	(included in) rfid_get_uid()	REQA and anti-collision combined
rfid_command(RFID_ANTI_COLLISION_LOOP)	(included in) rfid_get_uid()	See above
rfid_command(RFID_RATS)	rfid_transmit()	User needs in-depth protocol knowledge
rfid_command(RFID_APDU)	rfid_transmit()	See above
rfid_command(RFID_HLTA)	rfid_transmit()	See above

I see pros and cons on both sides. Fundamentally, the driver should handle as much of the work as possible while still remaining flexible. I think combining REQA and anti-collision makes sense. Or are there situations where one would need to call them individually?

As for RATS, APDU, etc., I’m personally not a fan of requiring rfid_transmit() in my API, as this demands deep protocol knowledge from the user. That’s why I initially find Alexander’s approach better.
However, even in his version, I would prefer combining certain functions for clarity and ease of use.

[alexanderwachter]

The main reason why I prefer a split into subsys and driver is to keep the driver as simple as possible.
It helps to keep code duplication at a minimum, makes it easier to maintain and makes testing much easier.
You could for example make an emul driver for the rfid driver and have posix tests for all the logic of the subsys.

[cmm1981]

The main reason why I prefer a split into subsys and driver is to keep the driver as simple as possible. It helps to keep code duplication at a minimum, makes it easier to maintain and makes testing much easier. You could for example make an emul driver for the rfid driver and have posix tests for all the logic of the subsys.

As I understand it, I have already defined a subsystem — namely with the functions: rfid_select_mode(), rfid_protocol_select(), rfid_get_uid()... My CR95HF driver then implements these functions. From my perspective, the only difference is that our interface definitions diverge slightly. The question now is which concept is the better one. Perhaps it would even be best to take the best parts from both concepts.

Since this is my first time working with RFID at all, it's difficult for me to make well-informed decisions. For my application, I simply need to read the UID, which I have managed to do with my approach.

Before I start a major refactoring effort, I would really appreciate hearing some more opinions. Otherwise, I would prefer to leave my driver as it is for now. Of course, I’ll address the last review findings and add documentation beforehand.

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
10.0% Duplication on New Code

See analysis details on SonarQube Cloud

[pdgendt]

So, this needs some more housekeeping, the commits should be created as per the contribution guidelines.

The major part that is missing here, is documentation. Introducing a new subsystem requires a dedicated documentation section to provide users with the hows/whys/whats/...
Also introducing a new subsystem will need to be presented at the Architecture Working Group at some point.

[alexanderwachter]

Definitely going in the right direction now!
What is also missing is a sample on how to use this. Maybe just a simple detection and print uuid.

[carlescufi]

@cmm1981 any chance you'd be available tomorrow at 17h CEST to discuss this in the Architecture WG meeting? otherwise next Tuesday perhaps?

[cmm1981]

@cmm1981 any chance you'd be available tomorrow at 17h CEST to discuss this in the Architecture WG meeting? otherwise next Tuesday perhaps?

Sorry, I did not read your message earlier. Next Tuesday is not possible for me. Tuesdays are unfortunately always difficult for me. I’ll try to join the following week.

[carlescufi]

@cmm1981 any chance you'd be available tomorrow at 17h CEST to discuss this in the Architecture WG meeting? otherwise next Tuesday perhaps?

Sorry, I did not read your message earlier. Next Tuesday is not possible for me. Tuesdays are unfortunately always difficult for me. I’ll try to join the following week.

OK. What I'll do is bring it up briefly tomorrow and then let you know of the outcome to see if we need your input. Would you be able to join on a Thursday at the same time?

[cmm1981]

@cmm1981 any chance you'd be available tomorrow at 17h CEST to discuss this in the Architecture WG meeting? otherwise next Tuesday perhaps?

Sorry, I did not read your message earlier. Next Tuesday is not possible for me. Tuesdays are unfortunately always difficult for me. I’ll try to join the following week.

OK. What I'll do is bring it up briefly tomorrow and then let you know of the outcome to see if we need your input. Would you be able to join on a Thursday at the same time?

Yes, Thursday would work better for me. Regarding the RFID API, @pdgendt could also be helpful since it’s a takeover from him. I adapted my CR95HF driver to his API definition.

[sonarqubecloud]

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.2% Duplication on New Code

See analysis details on SonarQube Cloud

[mbolivar]

Just a few inital comments. Thanks for your work!

I'm assuming you're going to squash the commits down and get it in a reviewable state by the time you're ready to take it out of RFC.

[mbolivar]

Aliases are generally only used by samples for things like LEDS, buttons, etc. that are shared across boards.

Suggest DT_NODELABEL(cr95hf) -- that aligns better with your example anyway.

[mbolivar]

Missing default y here?

[mbolivar]

Magic node label plus single instance? Needs fixing before we take this out of RFC.

[mbolivar]

E.g. note you have a multi instance emulator driver but a hard-code IRQ line -- that's a problem

[cmm1981]

Just a few inital comments. Thanks for your work!

I'm assuming you're going to squash the commits down and get it in a reviewable state by the time you're ready to take it out of RFC.

Thanks for the review! I’ll wait for the architecture review feedback, then push a final update and squash the commits.

[mbolivar]

I’ll wait for the architecture review feedback, then push a final update and squash the commits.

Do you mean you're discussing this in the architecture WG meeting and waiting for that discussion to settle down? If so, that makes sense, but if you're hoping for people to approve an architecture just from this RFC, it's a bit too messy for that in its current state, at least in my opinion.

[cmm1981]

I’ll wait for the architecture review feedback, then push a final update and squash the commits.

Do you mean you're discussing this in the architecture WG meeting and waiting for that discussion to settle down? If so, that makes sense, but if you're hoping for people to approve an architecture just from this RFC, it's a bit too messy for that in its current state, at least in my opinion.

Yes, that’s how I imagined it. What do you find too messy — the many changes that made it hard to follow, or the code in general?

[mbolivar]

What do you find too messy

There are a dozen commits in this RFC and most of them look like fixup commits. Please review the contribution guidelines in the zephyr documentation for what is expected from a clean history.

@carlescufi and I gave a shorter talk about this a few years ago if you don't have time for that: https://www.youtube.com/watch?v=mnVNLM7ynMQ