search

astuff / pacmod3

PACMod Board Rev 3 Driver
MIT License
24 stars 14 forks source link

Adding Engine Report #63

Closed as-snehagn closed 4 years ago

as-snehagn commented 4 years ago

Requires PR#50 on astuff_sensor_smsgs. Adding engine report message - JIRA#20 on pacmod_dbc.

JWhitleyWork commented 4 years ago

@as-snehagn The only problem I have at the moment is that we haven't heard from @nimig18 as to whether or not the PID report messages are actually used. Fay wanted to hear this before removing them. If they are used, we should keep the code in here to parse them and change their IDs to match the firmware. If they are not used, we should remove them from the driver all at once (instead of just the one that this PR removes).

as-snehagn commented 4 years ago

The problem is that the CAN_IDs for the PID4 and EngineReport messages are the same, which is why I removed only the PID4 message. I had asked @mlemm99 and he said it was outdated and out of compliance. If Kevin F's team can confirm, we can either leave, change, or remove these messages.

JWhitleyWork commented 4 years ago

@as-snehagn I'm aware of this part. The CAN IDs for those reports in the driver don't match the firmware anymore and those reports aren't documented in the DBC either. However, Fay was very adamant that we check with Imig before removing them from the driver. If he still wants them, we can just change the CAN IDs in the driver to match the firmware and then add a task to add them to the DBC.

nimig18 commented 4 years ago

@JWhitleyAStuff The PID reports were put in as debugging early in the development of the Lexus, they can be removed now.

JWhitleyWork commented 4 years ago

Thanks for the feedback, @nimig18! @as-snehagn - I'm putting in another PR to remove those. I'll pass it on to you in a minute. I would like to merge that prior to merging this.

JWhitleyWork commented 4 years ago

See #64.

JWhitleyWork commented 4 years ago

@as-snehagn Please rebase on master to fix the conflicts.

JWhitleyWork commented 4 years ago

@as-snehagn Please rebase on master to fix the conflicts.

Nevermind, I grabbed it real quick.