OpenSourceEBike / TSDZ2-Smart-EBike

Flexible OpenSource firmware for TongSheng TSDZ2 mid drive ebike motor
GNU General Public License v3.0
255 stars 131 forks source link

New Foundation #105

Closed leon927 closed 5 years ago

leon927 commented 5 years ago

Just a quick update with a pull request. Still WIP and a lot of things to finalize but it is going really well! Initial tests are great!

Solved a lot of unknown bugs so far. Not that noticeable but they were there. Also optimized several parts so they execute/calculate faster. The resolution/response seems to be much better just by improving calculations and parameters. Overall much safer, especially for future development. The entire controller code is also much simpler and improved in so many ways.

Still, this is not even an alpha version. Have more things to do so will develop for another few days.

leon927 commented 5 years ago

Haven't been able to check everything. In the future if you'd like more useful feedback/reviews, it'd be best to split these kinds of things out into separate pull requests. E.g. the renames of certain variables (wheel_max_speed <-> wheel_speed_max) could've been a single pull request that would've been really easy to review, and would remove a lot of the "noise" in this one.

When doing rapid development and trying to be effective with my time it is not that optimal to create separate pull requests for each and every step. Some things I am testing purely for development and these things will be removed. Making it very unnecessary to add in a pull request and discuss. Also, the more I discuss why I do things for the moment the more time I lose for development.

I am forever grateful for feedback but would appreciate it more when I am getting ready to release it to users. This is still in heavy development for at least a couple of days.

Some things and changes are made so the development on the new displays will be much easier for new developers. I would LOVE it if more developers would start developing for this great community so that we get support for more displays. But for this to happen I want the controller code to be the very best it can be. I for one would have nothing to do except develop display code if I am able to make this controller code "perfect".

Casainho has done a fantastic job with the foundation and my only goal is to improve where possible and in some way finalize the controller code. It will then be much simpler to develop the code for the displays.

Frans-Willem commented 5 years ago

I understand it's usually a hassle to split out commits and add proper commit messages, but if we'd like to get more developers involved, I think this is important.

From my perspective: I've got a few things I'd like to change, and I'd like to dive in and add them to the code. However the main branch has already diverged quite a bit from 19.0, and all of the commits are huge commits with changes all over the place, with mostly empty commit messages like "WIP".

How am I expected to figure out what's changed functionally, if it's even safe to run the main branch on my motor, or what's currently "in progress" and what's "stable" ? If I actually do start coding, how do you expect me to solve merge conflicts if I can't even find out why things were changed in the first place ?

So yeah, clean commits and commit messages definitely are a hassle, but they're invaluable when working together with other people. If it's only your own personal project, by all means do whatever works for you, but if we'd like to get other people involved, we need to make sure they have the information they need to actually get involved.

leon927 commented 5 years ago

The commits are huge as there are a lot of changes being made and some are not incorporated whilst others are. Many things are dependent so there is no chance of not changing a lot of things.

I also did not know that there are other people wanting to develop for the controller. I have been active for a couple of months now and there have been no developers, except Casainho, showing interest nor intent of doing some official changes and actual development.

I do agree that regular development should be with clear documentation. But in this case it is basically a complete rewrite on many things and should be handled as such.

If you are eager to develop would you mind being patient for the time being? Because a lot of things had to be changed in order for us to have multiple riding modes and functions. At the same time have an easy path for adding support for more displays.

If you look at the past you will see smaller pull requests but that is normal development. This is basically me trying to do the best I can to simplify everything so developers such as yourself can make changes more easily. The code has gotten a lot simpler so anyone can make changes without it causing strange bugs throughout.

Frans-Willem commented 5 years ago

I appreciate you trying to simplify things for others, but in the process you're making it very hard for me to pitch in. Basically the current master branch as well as this pull request is a combination of refactoring for simplicity, stuff that's still in progress, as well as new untested and undocumented features (some done, some not), all mixed together in an incoherent collection of commits :-/

I think for the time being, I'll just branch off of 19.0 and implement the fixes that I need. If you'd like other people to contribute on the main branch, I'd highly recommend to:

Furthermore, if simplicity is what you're after, it's paramount that you discuss code with others before committing to it. What may appear simple when you're knee-deep in the code and working on that specific feature, may not appear simple at all to someone else reading the code.

leon927 commented 5 years ago

Really sorry you feel that the rewrite I am doing to simplify and improve the entire code should be carried out in a different way. This is not to be compared to regular development.

Anyway, your plan for the time being is solid. Soon there will also be a beta and you are free to give feedback and suggestions then. Hopefully you will like the changes and the issues you reported will be solved or at least significantly reduced.

My intention is not to make it harder for you or in any way take away from your experience with this community, so I truly do feel sorry you feel that way.

Just want to add another reason I am rapidly developing and that is because everything should be somewhat finished with a good layout for the introduction of the new displays. Casainho has said he wishes to make a working prototype of the SW102 within a couple of weeks and I am trying to beat that deadline. I do not have that display at the moment but I know when it is time to develop to that display I would want to have the changes in the controller I have implemented so far.