drivers/mfrc522: add new driver #16782
Conversation
HendrikVE
requested review from
aabadie,
fjmolinas,
leandrolanzieri,
MichelRottleuthner and
smlng
as code owners
github-actions
bot
added
Area: doc
HendrikVE
added
the
Type: new feature
HendrikVE
force-pushed
the
pr/driver/mfrc522
branch
from
a744d1d to
b4d70f3
Compare
drivers/include/mfrc522.h
Outdated
| * @param[in] rx_align Defines the bit position in back_data[0] for the first bit received | ||
| * @param[in] check_crc True => The last two bytes of the response is assumed to be a CRC_A that must be validated | ||
| * | ||
| * @return MFRC522_STATUS_OK on success, MFRC522_STATUS_??? otherwise |
There was a problem hiding this comment.
Can you use ERROR codes instead according to doc.riot-os.org/driver-guide.html?
There was a problem hiding this comment.
I'm not sure whether I understand the doc correctly. Is there a errno.h where I should take these error codes from or should I declare them by myself but using the given naming scheme (and without an enum)? I doubt there is already a error code for a wrong crc value for example.
There was a problem hiding this comment.
I added error codes, namingly -EINVAL and -ENOTSUP to the device_read and device_write functions (they are just passed on from spi).
From the documentation: MUST: all error and status return codes are named, e.g. DEVAB_OK, DEVAB_NOSPI, DEVAB_OVERFLOW, ... I'd say mfrc522_status_code_t is following the rules. I really can't press status codes like MFRC522_STATUS_COLLISION or MFRC522_STATUS_CRC_WRONG into generic error codes. Especially because MFRC522_STATUS_COLLISION is used within the driver.
|
Here is some shell output: |
HendrikVE
added
the
CI: ready for build
There was a problem hiding this comment.
Thanks for your PR. I very interested in this driver for a personal project that I never found enough time for. I hope I can give you an detailed review next week.
Please check the RIOT driver programming guide that @fjmolinas kindly provided a link for. Also, there are a couple of automatic checks that run in the CI that check e.g. for tabs being used instead of spaces, multiple empty lines, etc. You can run those locally also with make static-test, but result might slightly differ when the versions of the tools used locally and in CI differ.
drivers/include/mfrc522.h
Outdated
| * | ||
| * @return MFRC522_STATUS_OK on success, MFRC522_STATUS_??? otherwise | ||
| */ | ||
| mfrc522_status_code_t mfrc522_pcd_transceive_data(mfrc522_t *dev, |
There was a problem hiding this comment.
Please use int as return type and 0 for success. For error codes please use negative errno codes. Refer to the driver guide for reasoning.
There was a problem hiding this comment.
I've read the documentation but I don't quite see how this is supposed to be compatible with some of these return values.
Whenever you implement status/error return values in your driver, magic numbers MUST NOT be used.
mfrc522_status_code_t are not magic numbers imo. A magic number would be to return a hardcoded -1, but the enum's members are well documented. To improve things I could replace the MFRC522_STATUS_??? otherwise-part of most functions by explicit naming of the possible return values to fulfill document every single error code that can be return for each function. The doc wants errno but does not give a reasoning for it as far as I can see.
Take MFRC522_STATUS_COLLISION for example. This does not perfectly fit into the scheme of classical errno numbers. Furthermore it is used internally by mfrc522_picc_select and it explicitly checks for the return of mfrc522_pcd_transceive_data which can be MFRC522_STATUS_COLLISION to do some collision specific handling within the driver. Using generic errno codes here does not only remove the context of the error and thus loosing important information for error handling about the result, it also could lead to problems once the same errno is used somewhere else where it does not mean collision but maybe something different. Yes, latter might be prevented by the doc attached to retval saying in which situation a given errno will be used exclusively, but that is still not a nice solution in my opinion. The only way I see at the moment to have both, internal status codes and errno codes for the user, would be to use a facade of functions which maps internal status codes to errno codes and make those functions with status code for internal use only.
After a quick scan of some random header files from drivers/include/ there seem to be way more drivers using custom error codes than doing the opposite, so I wonder how young this guideline is?
There was a problem hiding this comment.
The doc wants errno but does not give a reasoning for it as far as I can see.
Indeed, the reasoning is only given in the PR: #15018 Likely this also makes sense, as the driver guide purpose is more being a guideline than a reasoning of design decisions.
After a quick scan of some random header files from drivers/include/ there seem to be way more drivers using custom error codes than doing the opposite, so I wonder how young this guideline is?
September 2020, but I don't see how this is relevant. New drivers need to use errno codes, and some old APIs already have been converted. The rest will follow eventually.
There was a problem hiding this comment.
Also under the section Additional Sensor Driver Checklist stands MUST: all error and return values are named, e.g. DEVAB_OK, DEVAB_NODEV, .... I'm following this rule with returning values like MFRC522_STATUS_COLLISION. This driver is not really for a typical sensor, but also not that different in its way of working.
There was a problem hiding this comment.
It is also stated in General Device Driver Checklist:
MUST: all error and status return codes are named, e.g. DEVAB_OK, DEVAB_NOSPI, DEVAB_OVERFLOW, ...
There was a problem hiding this comment.
I missed the discussion about it in the matrix room but to me it feels like a usecase like this here might have been overlooked? I understand that it is desirable to have a unified result space for all drivers, but it also needs a place for special return values that are unique to a specific driver. In #15018 is stated:
The error classes that device driver have to report are largely the same
"Largely the same" is correct. Some return values simply don't fit perfectly into the scheme.
A lot of code within RIOT and in external code is doing so already. Applying this consistently allows passing error codes through, rather than translating between errno codes and custom enum values
The only difference here would be that I would need to translate it within the driver space instead of the application developer deciding.
There was a problem hiding this comment.
Note that while it would be super if applications would do proper error handling. But the reality is different. The majority uses the ostrich algorithm for error handling.
And they do have at least some merit (not at completely abandoning error handling, but at shifting most of the responsibility of error handling to the driver): The developer who is most capable to do error handling is the author of the driver, and the place to best do it is in the driver, as this is where most info about the device and driver state is freely available. Also, it dedulicates code if it is done at one place in the driver. So, IMO all errors that sensibly can be handled in the driver, should be handled there. (Usually that statement is uncontroversial, but what "sensible" is can be quite controversal from time to time.)
So errors that are reported back to the applications are those that cannot be sensible handled to the driver. And for applications it doesn't matter that much what exactly input/output error EIO exactly refers to.
In fact, I would be extremely happy if every application would consistently check every return value for a potential error (where applicable) and at least do some least effort recovery, such as three times resetting the driver and resetting the system if there were no "progress" after each and every of the three resets of the driver. And ideally the application would give proper info to aid debugging (being able to use strerror() makes that more likely to happen.) But most firmwares I have seen either ignore errors altogether, or treat it like being struck by a lighting and just melt down.
In internal APIs (please don't put them in the header in drivers/include but in driver/<module_name>/include ) there is much higher freedom, as you don't write this APIs to best match the needs of applications but rather what your driver needs. If you cannot reasonably map errors to errnos there, than don't.
There was a problem hiding this comment.
The point is that it is not as easy to decide which function is internal as one might think. I took the time to sweep over the relations of the functions and following are the results of it:
Functions with no internal use. We could map to errno within these functions.
- mfrc522_pcd_init
- mfrc522_pcd_antenna_off
- mfrc522_pcd_set_antenna_gain
- mfrc522_pcd_soft_power_down
- mfrc522_pcd_soft_power_up
- mfrc522_mifare_ultralight_write
- mfrc522_mifare_decrement
- mfrc522_mifare_increment
- mfrc522_mifare_restore
- mfrc522_mifare_transfer
- mfrc522_mifare_get_value
- mfrc522_mifare_set_value
- mfrc522_pcd_ntag216_auth
- mfrc522_mifare_set_uid
- mfrc522_mifare_unbrick_uid_sector
Functions with internal use. Some of these could map to errno, but not all of them (see further below for examples)
- mfrc522_pcd_calculate_crc
- mfrc522_pcd_transceive_data
- mfrc522_pcd_communicate_with_picc
- mfrc522_picc_request_a
- mfrc522_picc_wakeup_a
- mfrc522_picc_reqa_or_wupa
- mfrc522_picc_select
- mfrc522_picc_halt_a
- mfrc522_pcd_authenticate
- mfrc522_mifare_read
- mfrc522_mifare_write
- mfrc522_pcd_mifare_transceive
- mfrc522_mifare_open_uid_backdoor
- mfrc522_picc_is_new_card_present
- mfrc522_picc_read_card_serial
Use does not matter (no mfrc522_status_code_t return value)
- mfrc522_pcd_reset
- mfrc522_pcd_antenna_on
- mfrc522_pcd_get_antenna_gain
- mfrc522_pcd_stop_crypto1
- mfrc522_pcd_dump_version_to_serial
- mfrc522_picc_dump_to_serial
- mfrc522_picc_dump_details_to_serial
- mfrc522_picc_dump_mifare_classic_to_serial
- mfrc522_picc_dump_mifare_classic_sector_to_serial
- mfrc522_picc_dump_mifare_ultralight_to_serial
- mfrc522_pcd_perform_self_test
Pair of functions [returning | checking] MFRC522_STATUS_COLLISION
- [mfrc522_pcd_transceive_data | mfrc522_picc_select]
- [frc522_picc_request_a | mfrc522_picc_is_new_card_present]
Functions [returning | checking] MFRC522_STATUS_TIMEOUT
- [mfrc522_pcd_transceive_data | mfrc522_picc_halt_a]
- [mfrc522_pcd_communicate_with_picc | mfrc522_pcd_mifare_transceive]
- [mfrc522_pcd_authenticate | mfrc522_mifare_set_uid]
So if we want to have the functions returning mfrc522_status_code_t to be public (thus returning errno), we would need to write a wrapper for these which maps to errno for external use, while using the original only internally.
Alternatively we replace all status codes by errno (also internally). We replace MFRC522_STATUS_TIMEOUT with ETIMEDOUT and MFRC522_STATUS_COLLISION with whatever value, I really don't have a good candidate for it. And in this process we might combine several errors in to one single errno, because others wont fit to the context of the error. But just because e.g. MFRC522_STATUS_CRC_WRONG is only returned but currently not explicitly checked does not mean it will never be the case.
There was a problem hiding this comment.
I added a fixup commit which tries to map to errno. Disadvantages for the driver are:
- we lose all context information about
MFRC522_STATUS_MIFARE_NACKas it is mapped to generic-EIO - we lose all context information about
MFRC522_STATUS_CRC_WRONGas it is mapped to generic-EIO MFRC522_STATUS_COLLISIONwas replaced by-ECONNABORTEDwhich does not really fit but comes the closest I think
I don't want to mix status codes and errno, nor do I want to add wrapper functions, so I believe this is the best solution under the given circumstances.
|
Thank you for the future review on my PR btw :) If you want I can already squash my commits to make it easier for you to review? |
|
From my point of view, you can squash at will. Since github essentially runs a |
HendrikVE
force-pushed
the
pr/driver/mfrc522
branch
from
fd81432 to
05a6622
Compare
HendrikVE
force-pushed
the
pr/driver/mfrc522
branch
from
05a6622 to
16a8c09
Compare
HendrikVE
force-pushed
the
pr/driver/mfrc522
branch
from
1c8efd4 to
b87d247
Compare
@maribu @fjmolinas Are you ok with the changes in return codes provided by the fixup commit? |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
stale
bot
added
the
State: stale
HendrikVE
added
State: don't stale
HendrikVE
force-pushed
the
pr/driver/mfrc522
branch
from
75c8d62 to
79d3869
Compare
HendrikVE
force-pushed
the
pr/driver/mfrc522
branch
from
79d3869 to
dcc8edd
Compare
|
bors merge |
|
Build succeeded: |
|
Thank you all! |
Contribution description
This PR adds support for the MFRC522. It is quite common in the Arduino world and it is quite cheap. The driver connects to the MFRC522 via SPI and is heavily based on the Arduino driver available here. Basically it was ported, but with several improvements in readability and documentation.
Testing procedure
The given (manual) test provides single commands for some driver functions.