Skip to content

Conversation

@jimwhitelaw
Copy link
Collaborator

This change implements new calculation methods to the existing scaleFactor + Bias formula. Some of the formulas for calculating a result from the ELM327 response were incorrect. The new calculators have been verified with this wikipedia page and with this online OBD2 calculation tool.

Implementation Notes:

  • This change implements an overloaded conditionResponse() method that takes a function pointer as its only parameter. In addition to allowing for multiple calculation formulas to be implemented, it also provides a means to implement new calculators for custom PID scenarios without needing to modify the library itself.
  • The library remains backward compatible.
  • PIDs that did not need a new calculation method use the existing unmodified method provided in conditionResponse()
  • Also included are numerous small changes for clarity, spelling, buffer overflow safety and memory optimizations.
  • The changes have been tested with an emulator and calculations verified via the online tool mentioned above, but since so many methods are affected, it warrants additional review and testing before merging.
  • The changes made in PR (new) Added parseCANResponse() method to parse multiline CAN responses #281 are also included here, so if this is merged, that PR can be closed.
  • Included a new example/test program that loops through every PID in the service 0x01 list and queries it. Viewing of the debug output is used to verify the presence of any errors. Used with an emulator that can respond to each query with a valid response.

Potential future improvements include:

  • Moving the calculators to a separate file or class for easier maintenance. For the time being, I've kept with the single file convention for the library.
  • Refactoring to optimize out code that is now deliberately somewhat redundant in order to preserve backward compatibility with the previous implementation of conditionResponse() and its single calculation formula. This would be a breaking change.

@PowerBroker2
Copy link
Owner

Sorry for taking so long on this. Looks good to me - let's merge this and close the previous PR.

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.

2 participants