-
Notifications
You must be signed in to change notification settings - Fork 117
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
Value returned by supportedPIDS_xx_xx() methods is off by one bit. #211
Comments
Could possibly fix by doing the following: uint32_t ELM327::supportedPIDs_1_20()
{
processPID(SERVICE_01, SUPPORTED_PIDS_1_20, 1, 4);
return (((uint32_t)responseByte_3) << 24) |
(((uint32_t)responseByte_2) << 16) |
(((uint32_t)responseByte_1) << 8) |
((uint32_t)responseByte_0);
} What do you think? |
Alternatively, it could be simpler to fix by doing this: uint32_t ELM327::supportedPIDs_1_20()
{
processPID(SERVICE_01, SUPPORTED_PIDS_1_20, 1, 4);
return (uint32_t)response;
} |
Yes, I think the second method is the easiest way to fix these methods (isPidSupported() just uses "response" ignoring the conditioned output). However, it feels like a band-aid and I wonder if other methods that undergo a similar cast might also be affected? I'll need to do some more testing to see. Now that I think about it, I bet that the same thing happens with monitorStatus() but it goes unnoticed because we are only interested in the most significant byte and ignore the rest. |
… bit. PowerBroker2#211 Changes processPID() and conditionResponse() to return double instead of float to preserve all bits of _response_.
…f by one bit. PowerBroker2#211" This reverts commit 3d9a9d2.
… bit. PowerBroker2#211 Changes processPID() and conditionResponse() to return double instead of float to preserve all bits of _response_.
… bit. PowerBroker2#211 Changes processPID() and conditionResponse() to return double instead of float to preserve all bits of _response_.
… bit. PowerBroker2#211 Changes processPID() and conditionResponse() to return double instead of float to preserve all bits of _response_.
Fix for #211 Value returned by supportedPIDS_xx_xx() methods is off by one bit.
Fixed in 3.2.1 |
commit f501c2e Author: Jim Whitelaw <jimwhitelaw@gmail.com> Date: Fri Jan 5 02:30:53 2024 -0700 Update for fix for PowerBroker2#211
… bit. PowerBroker2#211 Changes processPID() and conditionResponse() to return double instead of float to preserve all bits of _response_.
commit f501c2e Author: Jim Whitelaw <jimwhitelaw@gmail.com> Date: Fri Jan 5 02:30:53 2024 -0700 Update for fix for PowerBroker2#211
Describe the bug
Value returned by supportedPIDS_xx_xx() methods is off by one bit compared to the value that is actually returned by the ELM327 device.
To Reproduce
Snippet of sample code used:
Expected behavior
The received response, conditioned response, and value returned from the method should all be the same. They are not. Debug output:
Potential cause
I believe the issue arises in conditionResponse() either where the uint64_t response is multiplied by float scaleFactor or when the conditioned value (float) is returned. I think that one of those implicit casts to float is causing the LSB of the response to be dropped. I'm not sure how to fix at the moment, perhaps someone else knows the answer.
The text was updated successfully, but these errors were encountered: