Skip to content
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

resolved mfrc522.PCD_Init() hang on teensy LC #344

Closed
wants to merge 1 commit into from
Closed

resolved mfrc522.PCD_Init() hang on teensy LC #344

wants to merge 1 commit into from

Conversation

ryangrahamnc
Copy link

mfrc522.PCD_Init() hangs on Teensy LC. Need a delay between the pinmode change and the read.

Q A
Bug fix? yes
New feature? no
Doc update? no
BC breaks? no
Deprecations? no
Fixed tickets

@Rotzbua
Copy link
Collaborator

Rotzbua commented Oct 30, 2017

		 		// Set the resetPowerDownPin as digital output, do not reset or power down.
		 		pinMode(_resetPowerDownPin, OUTPUT);

		 		if (digitalRead(_resetPowerDownPin) == LOW) {	// The MFRC522 chip is in power down mode.
		 			digitalWrite(_resetPowerDownPin, HIGH);		// Exit power down mode. This triggers a hard reset.

Probably this is a bug, hat the pin is set as output and we read from it. Shouldn't it be set first in input mode, read the value, then set it to output mode and write to pin.
@ryangrahamnc Please can you check this?

Suggested code:

		 		// Set the resetPowerDownPin as digital output, do not reset or power down.
		 		pinMode(_resetPowerDownPin, INPUT);

		 		if (digitalRead(_resetPowerDownPin) == LOW) {	// The MFRC522 chip is in power down mode.
		 		        pinMode(_resetPowerDownPin, OUTPUT);
		 			digitalWrite(_resetPowerDownPin, HIGH);		// Exit power down mode. This triggers a hard reset.

@Rotzbua Rotzbua closed this Oct 30, 2017
@Rotzbua Rotzbua reopened this Oct 30, 2017
@ryangrahamnc
Copy link
Author

I'm not sure.
As far back as this commit, it appears to always be set as an output pin:
cc1294a#diff-f4b1f3c5a6f32cebf62dae53df8a63d1R197

Unfortunately, I'm not all that familiar with the code. But based on this comment #269 (comment), I believe the intent is for the user to do their own reset call by setting the pin to low in their code and then calling PCD_Init(). In that case, it would make sense to make it constantly an output pin.

@Titantompa
Copy link

Yeah, it seems totally accurate to first make sure it is in output mode and then read it to see what the current state is.

The only problem I see with it is that there seems to be some terminology issues, the library doesn't actually do a hardware reset - it powers up the MFRC522 if it already was powered down by user code or chance but it does not make sure that the chip is reset before continuing.

This has kept me with a flakey experience of the MFRC522 since the ones I have often doesn't work at all and I have to reset the ESP8266s to get them to work. Making sure the MFRC522 is hardware reset properly seems to have solved those issues.

@@ -179,7 +179,7 @@ void MFRC522::PCD_Init() {
if (_resetPowerDownPin != UNUSED_PIN) {
// Set the resetPowerDownPin as digital output, do not reset or power down.
pinMode(_resetPowerDownPin, OUTPUT);

delay(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does yield() also solve the problem?

@Rotzbua
Copy link
Collaborator

Rotzbua commented Jan 11, 2018

I wont merge this commit, because it seems only a workaround for the miss configured pin. Would be nice to see a pull request fixing this. Instead of opening a new issue I let this pull request open for a while.

@Rotzbua Rotzbua added this to the 1.4.x milestone Jan 11, 2018
@Rotzbua Rotzbua added the help_wanted 💉 here you can help others or/and make pull requests label Jan 11, 2018
@Rotzbua
Copy link
Collaborator

Rotzbua commented Feb 9, 2018

@ryangrahamnc Can you test the current master? It should be fixed by #354.

@Rotzbua Rotzbua closed this Feb 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help_wanted 💉 here you can help others or/and make pull requests wontmerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants