firsttris / mfrc522-rpi

:key: Control your MFRC522 RFID Module with your Raspberry-pi and JavaScript
MIT License
118 stars 35 forks source link

Very long delay on startup: Reason for 250ms delay? #16

Closed trankin closed 4 years ago

trankin commented 4 years ago

https://github.com/firsttris/mfrc522-rpi/blob/c069e46ee3964312873d468683da85a2b19bf4c7/index.js#L202

I uncovered an issue when this device isn't actually connected, or if rpio is in mock mode.. this loop with Atomics.wait causes my program to hang for several seconds before this frees up.

Can you provide some insight into the reason for the 250ms delay and reducing that by 1ms per cycle for 250 cycles. I'm considering refactoring this, but want to have a better understanding first.

Thanks,

Thomas

firsttris commented 4 years ago

@AhmedBHameed added this line. could you tell if and why this is needed?

firsttris commented 4 years ago

@trankin if you find out this is not needed from your point of view. please share your thoughts. we are happy about any clarification. i have no idea why this was added. need to wait for feedback from @AhmedBHameed

trankin commented 4 years ago

I've got a fork that I refactored to 25ms instead of 250ms to match what was documented in the comments of the file. It doesn't appear to have any negative side effects, but I don't know what was intended, or what edge case this may have been handling...

AhmedBHameed commented 4 years ago

Hello @trankin,

I can't see any point to run the program without functional connected RFID card! note that I might be wrong here.

The app will hung till the loop ends but this is because of trying to find any signal from the device and expend time delay more for each loop for a another chance.

Let me explain more about the delay process.

The maximum time for delay is around 30 seconds in case of no signal received. BUT the delay start with 0 ms and start expanding for each cycle. The reason behind this is that most of IoT is working as synchronous to match the configuration of the manufacture and for that i used Atomics.wait since it will block the code till the respond come.

You can run the following code to see how Atomic.wait works here.

let int32 = new Int32Array(new SharedArrayBuffer(4));

// Start calculate the time required to finish the proccess.
const startTime = process.hrtime();

// Check the memory index 0;
console.log(int32[0]);

for (let i = 250; i > 0; i--) {
  const r = Atomics.wait(int32, 0, 0, 250 - i);
  console.log(`${i} =>> ${int32[0]}`);
  if (i === 200) {
    Atomics.store(int32, 0, 123);
    Atomics.notify(int32, 0, 1);
  }
}

// The time required til the end.
const finishTime = process.hrtime(startTime);
const nanoseconds = finishTime[0] * 1e9 + finishTime[1];
const seconds = nanoseconds / 1e9;
console.log(`CHECK=>>: finishTime at ${seconds} s`);

My knowledge was/is not enough to control the frequency clock using javascript to determine the exact time required. so i went with a exponential delay and leave the application take the time required for configuration.

I didn't care about the time value since the exponential delay was solving my issue at any time the configuration complete. I'm happy to hear that you tried to reduce it without any issues.

Good luck.

AhmedBHameed commented 4 years ago

Will close this issue. Feel free to reopen it.

firsttris commented 4 years ago

I can't see any point to run the program without functional connected RFID card! note that I might be wrong here.

I think he has build an application with this package and its running in the same node process. is my assumption correct @trankin ?

I mean whatever you would probably do with this package, you would probably add some code to the node process where the mfrc522-rpi is running.

and a delay that randomly blocks the node process for several seconds could create strange side-effects.

i found that we did not had any delay at this part of the application and it was added by you @AhmedBHameed in this commit https://github.com/firsttris/mfrc522-rpi/commit/ea37c177dd8bbbaa76d9a4d4a79b61076884f71e#diff-168726dbe96b3ce427e7fedce31bb0bcR202

could you elaborate more why you added this delay? i mean it was not there before, and i was not aware of any communication issues reported.

my understanding is you wanted to give the MFRC522 more time to get the respond from the RFID chip? but how do we know when this exact time is? i mean couldn't it be that the node process is still blocked and the respond is already rdy..?

would there not be another way without blocking the node process?

AhmedBHameed commented 4 years ago

Hello @firsttris,

TBH, I'm sure that I added this line for a reason but can't remember it unfortunately!!!

I did test the code without this line and do work perfectly so i follback to the priginal code and opened PR.

I Also did a test with the same module running while plugging out and plugging in all wires and still work perfectly [Note that i made RJ-45 for the connect for ease of use].

It is also important to mention that the module is Synchronous which means that has to be wrapped with asynchronous function like setTimeout or cron job for example.

Hope that also solve the issue for @trankin.

firsttris commented 4 years ago

hey @AhmedBHameed i really appreciate that you say it this honestly. not everyone is so honest. thx for your help.

@trankin might be already working on a new project :+1:

ill close this one

trankin commented 4 years ago

Yes, I've got this code integrated into another working application where the blocking causes unacceptable side effects where other asynchronous code and event loops stop operating until this method returns.

There are a number of reasons that would cause the device to be non functioning, one of the cases in my scenario is that I'm debugging and testing my code in a windows environment where the raspberry pi pins are mocked instead of real.

.. I can certainly code around this, and / or move this code into another process. I will likely refactor this library if possible to make the calls asynchronous instead of synchronous. <== I feel like asynchronous execution is more in the nature of the language in general.

I may be wrong in my ability to refactor this code, but .. any pitfalls I may encounter or insight would be appreciated.

Note, that I just read that you have a PR in to adjust this code back to the original scenario. I will say that I have a fork of this already that I have removed the offending line and it appears to also be working as expected in both the windows and linux (pi) environment.

Thanks,

Thomas

firsttris commented 4 years ago

@trankin the code was originally migrated from an arduino project. keep in mind we probably do not have more knowledge than you do!

if you have success in your async refactoring i would really appreciate a pull-request. you have to give something back 😏 🙌

best regards Tristan