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

Add support for Maxus Euniq 5 6-seat #861

Merged
merged 6 commits into from
Mar 28, 2023
Merged

Conversation

Jockz0rz
Copy link
Contributor

I have added support for Maxus Euniq 5 6-seat.
The code is mostly a copy of the code from Maxus eDeliver3 with minor edits.

Also this is my first time I submit a pull request so I hope I doing everything right.

@dexterbg
Copy link
Member

Jock,

welcome, very nice first contribution :-)

This is mostly perfect, except:

  1. You must not include generated files, in this case charts.js.gz in a PR, without the corresponding source file change that caused the change. I assume you actually didn't change the charts.js file, but accidentally got a slightly different .gz from the source by means of your build tools. You need to remove the .gz file from the PR / replace your file by the original version.
  2. You've kept the eDeliver config/metrics namespace xmg for the Euniq. You need to introduce a unique namespace for each vehicle, to keep config parameters and metrics distinct. Otherwise a user switching between these two vehicles (maybe because she/he uses the same OVMS unit for both) may run into conflicts, because configuration values and metrics have a different meaning for each. While that may not be the case yet, vehicles may evolve into different directions. You could maybe use xme or xmq, but I haven't verified these are unused.

Regards,
Michael

@Jockz0rz
Copy link
Contributor Author

Hi Michael,
Thank you for the input :-)
I think I have done everything according to you comments now.
But as I said it is my first time using git and contributed to a opensource project so any input is really appreciated.

@dexterbg dexterbg merged commit 7cff295 into openvehicles:master Mar 28, 2023
@dexterbg
Copy link
Member

When you're ready to announce the new vehicle support to all users, add an entry to changes.txt and a link to the vehicle in the README.md. It's also recommended to update the sdkconfig defaults in support.

Regards,
Michael

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.

None yet

2 participants