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

Write + read error in am2320 #1311

Closed
louisvangeldrop opened this issue Feb 22, 2024 · 9 comments
Closed

Write + read error in am2320 #1311

louisvangeldrop opened this issue Feb 22, 2024 · 9 comments

Comments

@louisvangeldrop
Copy link

Target device: NodeMCU ESP32c3-devkit

Description
I'am using the example from folder %MODDABLE%\examples\drivers\sensors\am2320

Steps to Reproduce

  1. Build and install the app using this build command: mcconfig -m -d -p esp32/c3_32s_kit
  2. I get a Write error at line 88:io.write(cBuf) in debug window:
                Timer.delay(10);
		
		// start conversion
		cBuf[0] = Register.COMMAND_READ;
		cBuf[1] = reg;
		cBuf[2] = 0x02;
         	io.write(cBuf);
		Timer.delay(2);

I have looked at the Adafruit am2320 driver: https://github.com/adafruit/Adafruit_AM2320/blob/master/Adafruit_AM2320.cpp
and changed the delay-duration in Timer.delay(10+100). As a result I don't get the write error.

However I also have to change the Timer.delay(2) into Timer.delay(2+5) to avoid an io.read error.

@louisvangeldrop
Copy link
Author

I have changed the "wake" code just like the Adafruit-code into:

               for (let i = 0; i < 3; i++) {
			try {
				io.write(new ArrayBuffer(8));	// write 8 length buffer
				written = true
				break
			}
			catch (e) {
				Timer.delay(100)
			}
		}

That works.

@phoddie
Copy link
Collaborator

phoddie commented Feb 22, 2024

I guess @mkellner didn't encounter the read/write failures that you are experiencing when he wrote the original driver.

100 ms is a looong time to wait. I quickly checked the data sheet and it seemed to indicate a shorter time should be enough. Does it fail with shorter times (like 10ms)?

Would you like to make a PR to merge this change?

@louisvangeldrop
Copy link
Author

I have changed 100 ms in 10 ms and it works.

@louisvangeldrop
Copy link
Author

BTW I have no idea how make a PR. Maybe I am too old for it.

@phoddie
Copy link
Collaborator

phoddie commented Feb 22, 2024

No problem. I've merged your changes. They were incomplete (how written is handled is omitted from the snippet above), so I made a guess. I also did some cleanup on the driver, since it used some older coding patterns.

The full source is on this gist. Could you give that a try? I confirmed it compiles but I don't have the sensor to confirm it actually works. Once you confirm that, I'll commit it. Thanks!

@louisvangeldrop
Copy link
Author

Tests running last night and today showed that the 10 ms delay is too short. Sometimes the temperature or humidity value is undefined. With a 100ms delay I do get twice a "catched" exception and with 50 ms three times. Both delays produce correct temperature and humidity values.

@phoddie
Copy link
Collaborator

phoddie commented Feb 23, 2024

Thanks for giving it a try. The timing / retry behavior seems like a mysterious incantation. Since I don't really understand it, it seems safest to duplicate the Adafruit approach. I've updated the gist to do that. Does that work for you?

@louisvangeldrop
Copy link
Author

It works. Thx for the help

@phoddie
Copy link
Collaborator

phoddie commented Feb 24, 2024

Cool, thanks. Will get that committed.

FWIW – the delays are long enough to be a bit annoying. They exist because the sensor automatically powers down to save power if the sampling interval is high enough. An alternative would be to use a repeating timer to keep the sensor active. There could be an option to disable that, but at least the default behavior would be more-or-less non-blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants