Skip to content

Fixes and new features #23

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

Open
wants to merge 14 commits into
base: Master
Choose a base branch
from
Open

Fixes and new features #23

wants to merge 14 commits into from

Conversation

MartGB
Copy link

@MartGB MartGB commented Jan 10, 2022

I thought I would test the master branch and the one thing I changed from my local branch, screwed the current target calculation up, now it works again.
I am testing the field weakening now, but I didn't want to have a bug in the master code, so that is why I am already creating a second pull request now.
I added the CRC for my LCD8.
Also added and tested cruise on LCD8.
And improved the battery current calculation for the LCD, which produced overflow before on regen.
It starts in sine wave mode now when the eeprom is reset during programming.

Mart GB and others added 12 commits January 10, 2022 11:50
… the position when enabled, also disables on braking or assistlevel switching or pedalling or throttle position is higher than lowest value during cruise
…hich fixes the throttle to the position when enabled, also disables on braking or assistlevel switching or pedalling or throttle position is higher than lowest value during cruise
@MartGB
Copy link
Author

MartGB commented Jan 10, 2022

The features I added are also tested and working by the way.

@MartGB
Copy link
Author

MartGB commented Jan 10, 2022

I tested field weakening and it works for me

@Xnyle
Copy link
Collaborator

Xnyle commented Jan 10, 2022

regarding removing a couple of characters... That's exactly what I tried to get rid of two years ago, "hacked" code snippets. If there are different options one would prefer or not they should be enabled at runtime (or if you don't have the app) at flash time, not at compile time.

For instance there are some changes regarding temp sensor. The temp sensor was working perfectly fine you just had to set two variables via app/config to configure the value range?

Anyway as I can't test or review anything right now, If the PRs escalate I can always just fork he last known version for my own use ;-)

@MartGB
Copy link
Author

MartGB commented Jan 10, 2022

I just saw that multiple people asked for changes I made albeit some time ago. That is why I am creating these pull requests. I do not have any idea how to add it to the configurator or app and the way it is now is fine for me.
Temp sensor was not working for me, since there wasn't any value assigned to i8_motor_temperature anywhere in the code.

@Xnyle
Copy link
Collaborator

Xnyle commented Jan 11, 2022

I think that variable is just a remnant as it is working via x4 (IIRC).

"is now is fine for me" that's exactly my point.

@stancecoke a better approach would maybe to just revert the changes for now from master but add a note at the top of the readme to @MartGB 's repo/fork so people know that there is further developement.

@MartGB
Copy link
Author

MartGB commented Jan 11, 2022

@Xnyle @stancecoke Or just remove commit 59aadbb and dde974f if you care about not having commented pieces of code.

@stancecoke
Copy link
Owner

@Xnyle, @MartGB: To be honest, I'm not interested in the Kunteng controllers any longer. I could move the master back to the origin at https://github.com/OpenSourceEBike/BMSBattery_S_controllers_firmware, or to any any other user who is interested to maintain this project...

@MartGB
Copy link
Author

MartGB commented Jan 11, 2022

@stancecoke What are you running as controller on your ebike now then? Or don't you have an ebike any longer?

@stancecoke
Copy link
Owner

@MartGB I've developed an open firmware for the Lishui FOC controllers meanwhile. They use a STM32F103 processor and are able to do real FOC.
https://github.com/ebics

https://github.com/EBiCS/EBiCS_Firmware/wiki/Where-to-buy-a-suitable-controller

@MartGB
Copy link
Author

MartGB commented Jan 11, 2022

@stancecoke Aah nice there is something out there with stm32 for extra processing power. There are not any high powered versions of the lishui I see. I am running 3kw peak. But for now this is good enough and smooth enough for me as I am coming from a 18fet stock square wave controller haha.

@stancecoke
Copy link
Owner

stancecoke commented Jan 11, 2022

@MartGB, @Xnyle @casainho : I've just merged Andreas fork to the master, so it will be some work to resolve the conflicts before merging this pull request. I'm searching for a new owner for this project, is one of you interested?!

@MartGB
Copy link
Author

MartGB commented Jan 12, 2022

@stancecoke What does being owner of this project entail?

@casainho
Copy link

I am not interested as I am not using anymore this controllers and motors. If no one want to maintain it, then I would put this repository in archive mode, so no new issues and pull requests will happen. And make clear on README is is archived without maintenance. Anyone will be able to fork and use it as a fork.

@stancecoke
Copy link
Owner

stancecoke commented Jan 12, 2022

@stancecoke What does being owner of this project entail?

@MartGB Just like I'm doing recently, decide if pull requests from other developers can be accepted and merged to the master or not. Nothing more. And of course you can decide, if your own developments are stable enough to be merged to the master, or if they should stay in a branch until they are prooved to be OK....

@Xnyle
Copy link
Collaborator

Xnyle commented Jan 12, 2022

@stancecoke not me as I currently neither have the setup nor the time to continuously test changes. I'll probably just maintain my own fork, cherry picking stuff I like.

@AkbarRamzan
Copy link

I am still interested in KT controllers, especially with LCD8H and field weakening. I am going to flash my controller soon so will be able to test these features.

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.

6 participants