Skip to content

Revert to using CLU15 message for Gear Selection for Compatability Reasons#362

Merged
rbiasini merged 38 commits intocommaai:develfrom
emmertex:hyundai-dev
Oct 14, 2018
Merged

Revert to using CLU15 message for Gear Selection for Compatability Reasons#362
rbiasini merged 38 commits intocommaai:develfrom
emmertex:hyundai-dev

Conversation

@emmertex
Copy link
Copy Markdown

Issues with using LVR12 message are as follows.

On 8 Speed, if in drive, but switched to "sport" mode, it no longer see's drive
On vehicles equipped with paddles for gear selections, pressing a paddle is seen as leaving drive.
Hyundai Eleantra does not create this message.

None of these issues are present when using the CLU15 message, while still allowing the same featureset.

Didn’t realise this was removed as well!
@jfrux
Copy link
Copy Markdown

jfrux commented Sep 12, 2018

Umm okay... but…. Why?
"Sport" mode really isn't for the openpilot world. Is it really called “Sport” mode?… or are you talking about “Sequential” which is like a paddle-shifted mode?

I’m not sure that it makes sense for OP to allow usage in Sequential mode on any car. Sequential mode isn’t to be used ever unless well… you think you’re somehow benefiting from shifting the car manually… of which you probably picked the wrong car to begin with and probably think self-driving cars are silly because IMA CONTROL MAH OWN CAR CUZZ I’M MAN AND MAN CONTROL CAR AND MACHINE CAN SUCK IT.

Also, anything other than drive is no place for OP... ever. Unless you're saying, OP is seeing it as being in drive... of which, you could be on to something!

Then again... what do I know? I'll tell you... nothing. Carry on.
:) <3

@emmertex
Copy link
Copy Markdown
Author

emmertex commented Sep 12, 2018

Because autos don't downshift properly on hill descents.

Feel free to provide fixes to the auto code base, but at this stage that is out of scope of this 'driver aid'

@jfrux
Copy link
Copy Markdown

jfrux commented Sep 12, 2018 via email

@emmertex
Copy link
Copy Markdown
Author

It's an ACC issue.
Because ACC will ride the brakes for almost an entire descent, you will have no brakes at the bottom. Everyone dies.
If you force 1 gear downshift, ACC will ride the brakes 1/2 as much, you will have brakes at the bottom.

There is no other reason OP can't fully drive those roads, but a forced downshift will force a manual drive.

The auto is happy to sit at 3000rpm for ascent, but won't auto down shift to allow more than 2250rpm on descent. The downshift is not sufficient on these vehicles.

OP should not be restricted due to bad auto design.

@rbiasini
Copy link
Copy Markdown
Contributor

@emmertex , As we discussed, can you please keep the Santa Fe the way it was? Ideally, I prefer listening to the gear from the engine module rather than the CLU, so we should find that source on all Kia/hyundai

Add logic to use transmission message rather than cluster message for all gear selections UNLESS the car in question does not have the known good transmission messages
@emmertex
Copy link
Copy Markdown
Author

Without actual access to an Elantra (or possibly another model with the same issue/transmission) this is the best I can do.

So since that is the only known ‘issue’ vehicle at this stage I am reading both cluster and transmission in carstate.py, and defining which one is to be used in interface.py with an if statement.

Please note the last 2 commits are not tested so please review for typo’s and mistakes.

Comment thread selfdrive/car/hyundai/interface.py Outdated

# gear shifter
ret.gearShifter = self.CS.gear_shifter
if candidate == CAR.ELANTRA:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

self.candidate?

@rbiasini
Copy link
Copy Markdown
Contributor

if somebody can test this on a Elantra and a non-Elantra, I'll merge it.
Thanks!

@TK211X
Copy link
Copy Markdown
Contributor

TK211X commented Sep 27, 2018

Completed test on 2017 Elantra Ultimate with Legacy Wiring

Brief of test:

Checked out and pulled latest Hyundai-Dev branch (#362)

OP loaded with no issues. Initial drive caused "Steering Temporarily Unavailable" on attempt to engage.

  • Attributed to legacy wiring.

Modified carstate.py to reflect this:
Line#121 return CANParser(DBC[CP.carFingerprint]['pt'], signals, checks, 2)
to --> return CANParser(DBC[CP.carFingerprint]['pt'], signals, checks, 1)

Rebooted

  • OP successfully engaged on following drive with no errors.

However, steering was found to still be unreliable and sporadic. Updating Interface.py with latest tuning values from Community branch resulted in no changes to aggressive OP steering behavior.
This issue is not present in Community branch.

Update for Elantra on Hyundai-Dev Branch
@emmertex
Copy link
Copy Markdown
Author

Merged changes made by TK211X
Tested on Sorento as well for non-elantra test

@rbiasini
Copy link
Copy Markdown
Contributor

@emmertex, if you give the green light (tested on Elantra too) I would like to merge this.

@emmertex
Copy link
Copy Markdown
Author

emmertex commented Oct 11, 2018 via email

@rbiasini rbiasini merged commit 5641fc9 into commaai:devel Oct 14, 2018
emmertex added a commit to emmertex/openpilot that referenced this pull request Oct 23, 2018
…asons (commaai#362)

* hyundai WIP

* steer_driver_factor is 1

* removed unnecessary file

* removed unnecessary code

* Update carcontroller.py

bug fix

* safety tuning and fixed interface stiffness

* better lateral tuning, some fixes

* Fix set speed

* added camera state reading, autoresume from stop, cancel on accel, hud alerts

* WIP

* Updated for Kia Sorento *WIP*

* Cleanup

* clean2

* Bug Fixes

* pre-merge

* Add all the cars!

* Panda to auto-detect Camera Bus

* Move Checksum Check

* Final Sorento Tuning

* Make CAN3 for Cam default

* Update README.md

* update panda, minor aesthetic updates

* few other minor changes

* added steer not allowed alert

* bup panda version to force panda update

* fixed camera alerts

* Revert to using CLU15 for Gear Selection

* Missing Defines

Didn’t realise this was removed as well!

* Change Gear Selection Definitions

Add logic to use transmission message rather than cluster message for all gear selections UNLESS the car in question does not have the known good transmission messages

* Revert Camera CAN Bus

* self.candidate

* fixed fingerpint, tested on Sorento

* Update for Elantra on Hyundai-Dev Branch

Latest Elantra values that correlate to progress Community branch.
emmertex added a commit to emmertex/openpilot that referenced this pull request Oct 23, 2018
klaus385 pushed a commit to klaus385/openpilot that referenced this pull request Dec 31, 2018
…asons (commaai#362)

* hyundai WIP

* steer_driver_factor is 1

* removed unnecessary file

* removed unnecessary code

* Update carcontroller.py

bug fix

* safety tuning and fixed interface stiffness

* better lateral tuning, some fixes

* Fix set speed

* added camera state reading, autoresume from stop, cancel on accel, hud alerts

* WIP

* Updated for Kia Sorento *WIP*

* Cleanup

* clean2

* Bug Fixes

* pre-merge

* Add all the cars!

* Panda to auto-detect Camera Bus

* Move Checksum Check

* Final Sorento Tuning

* Make CAN3 for Cam default

* Update README.md

* update panda, minor aesthetic updates

* few other minor changes

* added steer not allowed alert

* bup panda version to force panda update

* fixed camera alerts

* Revert to using CLU15 for Gear Selection

* Missing Defines

Didn’t realise this was removed as well!

* Change Gear Selection Definitions

Add logic to use transmission message rather than cluster message for all gear selections UNLESS the car in question does not have the known good transmission messages

* Revert Camera CAN Bus

* self.candidate

* fixed fingerpint, tested on Sorento

* Update for Elantra on Hyundai-Dev Branch

Latest Elantra values that correlate to progress Community branch.
@emmertex emmertex deleted the hyundai-dev branch August 17, 2019 01:18
ghost pushed a commit to dragonpilot/dragonpilot that referenced this pull request Apr 7, 2020
…asons (commaai#362)

* hyundai WIP

* steer_driver_factor is 1

* removed unnecessary file

* removed unnecessary code

* Update carcontroller.py

bug fix

* safety tuning and fixed interface stiffness

* better lateral tuning, some fixes

* Fix set speed

* added camera state reading, autoresume from stop, cancel on accel, hud alerts

* WIP

* Updated for Kia Sorento *WIP*

* Cleanup

* clean2

* Bug Fixes

* pre-merge

* Add all the cars!

* Panda to auto-detect Camera Bus

* Move Checksum Check

* Final Sorento Tuning

* Make CAN3 for Cam default

* Update README.md

* update panda, minor aesthetic updates

* few other minor changes

* added steer not allowed alert

* bup panda version to force panda update

* fixed camera alerts

* Revert to using CLU15 for Gear Selection

* Missing Defines

Didn’t realise this was removed as well!

* Change Gear Selection Definitions

Add logic to use transmission message rather than cluster message for all gear selections UNLESS the car in question does not have the known good transmission messages

* Revert Camera CAN Bus

* self.candidate

* fixed fingerpint, tested on Sorento

* Update for Elantra on Hyundai-Dev Branch

Latest Elantra values that correlate to progress Community branch.
mlocoteta pushed a commit to mlocoteta/openpilot that referenced this pull request Apr 1, 2021
* End 2 end lat toggle

* bool values don't have quotes

* strip whitespace

* Clean up

* Clean up

* Clean up

* Fix

* fix

* KL model
carleeno pushed a commit to carleeno/openpilot that referenced this pull request Aug 2, 2024
* MADS: HKG CAN-FD: Disallow cruise buttons to engage when pcmCruiseSpeed is off

* handle states properly

* this is better

* make sure main button is pressed

* oops flipped main

* same behavior

* handle cruise main button universally

* both

* not needed

* oops

* wtf how did i miss
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.

4 participants