Skip to content
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

Nissan port #1104

Closed
wants to merge 22 commits into from
Closed

Nissan port #1104

wants to merge 22 commits into from

Conversation

avolmensky
Copy link
Contributor

Nissan port. Should be pretty much complete. Open to comments/suggestions

@rbiasini
Copy link
Contributor

@avolmensky can you add a route to test_car_models.py? we'll take care of uploading the route so the regression test will pass.

@rbiasini
Copy link
Contributor

@avolmensky , the port looks pretty clean. I recommend re-basing with master and apply the changes to make it look like the other ports. There has been quite a bit of work during the last 2 weeks to simplify the car port code and those changes should apply to this port too before it gets merged. After those changes, this PR should be 100-150 lines shorter.

@pd0wm , once all the tests pass and the changes mentioned ^^ are done, I recommend merging this PR into openpilot and force it in dashcam mode (like Ford) until verified.

@rbiasini
Copy link
Contributor

rbiasini commented Mar 4, 2020

@avolmensky , not that you can preserve routes from useradmin, you should be able to fix the failing tests I think.

@rbiasini
Copy link
Contributor

rbiasini commented Mar 4, 2020

A couple of minor comments and tests that need to be fixed, then we should merge this PR in dashcam only mode. Almost there :)
Next step for official support would for comma to test the port on a car.

Copy link
Contributor

@rbiasini rbiasini left a comment

Choose a reason for hiding this comment

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

this PR looks good to me. Verified the process_replay segment has indeed an engagement event.

@pd0wm
Copy link
Contributor

pd0wm commented Mar 5, 2020

Thanks for the work on the port! Comma will rent a Nissan soon and do some final testing.

Don't worry about conflicting files, we can take it from here.

@pd0wm pd0wm closed this Mar 5, 2020
@tongclement
Copy link

Dear all, I would like to adapt open pilot for a 2013 Nissan Serena. Will this port.py file help me in any way? I am a beginner and new to the community so sorry if I am commenting at the wrong place. I will delete if this is in-apropriate. Many thanks

Clement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants