Skip to content

Include the last frame byte in the checksum calculation #16

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bloveless
Copy link

@bloveless bloveless commented Sep 13, 2020

Fixed checksum to include the last byte which is the temp H byte.

I was getting calculations such as the following.

Starting checksum: 178
0: adding 207 - new checksum: 129
1: adding 0 - new checksum: 129
2: adding 11 - new checksum: 140
3: adding 35 - new checksum: 175
4: adding 208 - new checksum: 127
----------------------------------------
i: 0: 207
i: 1: 0
i: 2: 11
i: 3: 35
i: 4: 208
i: 5: 9
i: 6: 136
127 = 136

You'll notice that the final checksum is 127 rather than 136 and that when calculating the checksum that frame[5] is missing. I've changed the checksum calculation to include this final byte.

After a little research, it seems that the tfmini and the tfmini-s use the same number of bytes in the frame but the tfmini-s use the last two bytes for the temperature. Either way, this change makes this library compatible with the tfmini-s as well as the tfmini.

Fixed checksum to include the last byte which is the temp H byte
Bump the patch version to include the latest checksum modification.
@PeterAJansen
Copy link
Contributor

Hi bloveless, thanks for this contribution. I don't have a tfmini-s handy, is this tested and verified working on both the tfmini and tfmini-s? My workbench is a little crazy right now (I've been taking care of our toddler during quarantine for 6 months), so it's not easy for me to test right now, and I don't want to risk breaking the driver if it's not yet tested on both.
thanks,
Peter

@bloveless
Copy link
Author

I don’t have a TFMini but after I got this driver to work I realized that only the distance measurement frame is the same. None of the commands work on the TFMini-S. So I’ll leave it up to you if you want to close this PR. I’m going to make a new driver for the TFMini-S based on your driver.

@bloveless
Copy link
Author

But thank you for making this driver! It has really made my development easier for the TFMini-S driver!

@PeterAJansen
Copy link
Contributor

Thanks -- I've had a few requests for the TFMini-S, but given how crazy the world is right now haven't had the time to pick one up and do the verification.

I think the changes are fairly minor to have the same codebase work on both (with a #DEFINE or similar that picks out which version to use) -- but maybe the best thing is to break it out into two separate drivers so that new users have something that just works out of the box without it being potentially confusing to have to set the define.

@bloveless
Copy link
Author

I'll try and remember to come back here when I'm done with my driver and maybe we can team up to provide a unified driver for both! Either that or we can just crosslink the projects.

@bloveless
Copy link
Author

I got most of the tfmini-s driver working last night if you want to take a look. https://github.com/bloveless/tfmini-s

@Bjohnson131
Copy link

can this be pulled? @PeterAJansen

@bloveless
Copy link
Author

@tuskiomi If you are trying to support the TFMini-S as I was trying to do with this PR then you might try out my driver specifically for the TFMini-S. There are a few more modifications that are necessary to support TFMini-S correctly and this change only allows you to read the height on a TFMini-S. None of the management commands work even if this PR is applied.

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.

3 participants