search

sandeepmistry / arduino-LoRa

An Arduino library for sending and receiving data using LoRa radios.
MIT License
1.63k stars 626 forks source link

Version 0.7.1 Resetting REG_FIFO_ADDR_PTR in LoRaClass::handleDio0Rise #353

Closed notascooby closed 4 years ago

notascooby commented 4 years ago

Not receiving Lora communication (interrupt driven) with ver 0.7.1 but works perfectly with ver 0.7.0. in ESP32 based device. Sketch & device have not been changed for over a year. With reference to issue #218 At that time I had the same issue and commented out LINE 583 in version 0.5.0. writeRegister(REG_FIFO_ADDR_PTR, 0); After commenting that line out, all worked perfectly. In later versions released up to and including 0.7.0 the above line of code had been removed in the commits and library worked great. In version 0.7.1 this line of code has been re-introduced (line 680) By commenting this line 680 out, interrupt driven receive works well. Not sure if the line has been accidently re-introduced, or there is a reason for adding it back in, but hopefully this information may be of use. I would prefer to use libraries as released without changes so if line 680 should be present, any pointers to the possible reasons for the issue would be gratefully received.

morganrallen commented 4 years ago

Yeah looks like this was reintroduced by PR #320 @ricaun was this a mistake or for a specific reason?

ricaun commented 4 years ago

Yes it was my mistake, the PR #320 was not supposed to change the Receive part of the handleDio0Rise, just add all the original code inside the if ((irqFlags & IRQ_RX_DONE_MASK) != 0) { I was looking whats change on the PR and what the hell I did, let's rollback this part and I gonna send a new PR fixing this part. 😢

morganrallen commented 4 years ago

No worries on rolling back, I'll just push a new change. What probably happened was you had an old version of master that didn't include this removal.

morganrallen commented 4 years ago

I've just release 0.7.2 which fixes this issue. Can take an hour or so for the release to appear, please close this issue when you think it's good @notascooby

ricaun commented 4 years ago

Perfect, the result is the same. Removing the line have the same result.

notascooby commented 4 years ago

Wow fast response - thanks. Version 0.7.2 solves the issue.

morganrallen commented 4 years ago

heh yeah, got all the time in the world these days.... and thankfully an easy fix for something that should have been caught, I'll see if I can implement some checks to help prevent things like this is the future.

IoTThinks commented 4 years ago

@morganrallen may be you can put a comment like // PR XYZ - Resetting REG_FIFO... in front of the code line.

So when you merge, if new code doesn't have this line, you will know. It is common in Software Development / Outsourcing.