Skip to content

Allow uart to go ~100Hz versus 10Hz #50

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

Merged
merged 6 commits into from
Jul 7, 2020
Merged

Allow uart to go ~100Hz versus 10Hz #50

merged 6 commits into from
Jul 7, 2020

Conversation

evaherrada
Copy link
Collaborator

So I tried a bunch of different ways involving checking how many bytes were in waiting, but in the end, just making the time.sleep smaller and adding a try/except loop was the fastest and most stable way to do it.

@evaherrada evaherrada requested a review from caternuson June 29, 2020 21:26
@tannewt
Copy link
Member

tannewt commented Jun 30, 2020

Why have the sleep at all? Why not just wait for two bytes?

@evaherrada
Copy link
Collaborator Author

@tannewt
So I tried doing that with:

while self._uart.in_waiting < 2:
    pass

And it didn't work. It kept getting caught for some reason.

@tannewt
Copy link
Member

tannewt commented Jul 1, 2020

You may need to add a timeout so that if it doesn't work you can escape the loop. Basically track how long has passed since you started and exit the loop if it is too long.

@evaherrada
Copy link
Collaborator Author

Ah, ok, that makes sense. I'll do that.

@evaherrada
Copy link
Collaborator Author

@tannewt So, I added the timeout, and I also just added a copy of _read_register that only gets run if there would have been a UART read error. Just adding that bit of recursion has made the UART mode way more stable.

@evaherrada
Copy link
Collaborator Author

It is slightly janky, and if you've got a cleaner way to do it, let me know because I definitely think this could be a lot cleaner.

resp = self._uart.read(self._uart.in_waiting)

if resp[0] != 0xBB: # Recursion
Copy link
Member

Choose a reason for hiding this comment

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

This is not recursion because recursion would call _read_register. Why did you add this? It wasn't needed before.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My initial plan was to actually do that, but I couldn't think of a good way to add in an exit condition so it didn't just keep going until it reached the recursion limit, and I guess I forgot to remove the comment.

It's needed because the, when running in UART mode, even in the version that's currently on master, it raises a UART read error every 3 or 4 reads when you're trying to use the simpletest. I think that before it may have mostly been tested by getting a single value, such as acceleration, at a time. That being said, even though I know the majority of people aren't using this with UART, I am quite surprised that I haven't seen anyone else encountering that issue, so it may be worthwhile for some other people with the sensor to do some testing to ensure that issue doesn't only affect me.

Copy link
Member

Choose a reason for hiding this comment

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

Do the errors go away if you increase the timeout? If you really need it, I think you could just have an outer loop and count how many tries you want to do before raising an error. No need to copy things a second time.

resp = self._uart.read(self._uart.in_waiting)

if resp[0] != 0xBB: # Recursion
Copy link
Member

Choose a reason for hiding this comment

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

Do the errors go away if you increase the timeout? If you really need it, I think you could just have an outer loop and count how many tries you want to do before raising an error. No need to copy things a second time.

@evaherrada
Copy link
Collaborator Author

evaherrada commented Jul 3, 2020

Ok, I think I'll do that.

They don't. It seems it just gets stuck and the only way to get it unstuck (without using a try/except in code.py) is to re-request the data from the sensor.

@evaherrada
Copy link
Collaborator Author

@tannewt Yeah, this works way better now. I felt confident enough in it to lower the timeout time to 0.1 seconds from 0.25 and I let it run for a while and it hasn't given me any issues.

@evaherrada evaherrada requested a review from tannewt July 3, 2020 21:08
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Two minor things. Looks good otherwise. Thanks!

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@tannewt tannewt merged commit d8ad1e6 into master Jul 7, 2020
@evaherrada evaherrada linked an issue Jul 10, 2020 that may be closed by this pull request
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 11, 2020
Updating https://github.com/adafruit/Adafruit_CircuitPython_BNO055 to 5.0.2 from 5.0.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_BNO055#50 from adafruit/uart_speed

Updating https://github.com/adafruit/Adafruit_CircuitPython_BLE_BroadcastNet to 0.10.1 from 0.10.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_BLE_BroadcastNet#12 from adafruit/discord-fix
  > Fixed discord invite link

Updating https://github.com/adafruit/Adafruit_CircuitPython_ImageLoad to 0.11.4 from 0.11.3:
  > Fix README rendering

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Added the following libraries: Adafruit_CircuitPython_ImageLoad
@evaherrada evaherrada deleted the uart_speed branch January 27, 2022 18:07
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

Successfully merging this pull request may close these issues.

UART mode only allows for 10Hz data reads
2 participants