Skip to content

Dev #79

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

Merged
merged 43 commits into from
May 18, 2021
Merged

Dev #79

merged 43 commits into from
May 18, 2021

Conversation

maxlem
Copy link
Contributor

@maxlem maxlem commented May 12, 2021

this contains my other MR which askuric says he's fine with it

#75

let me know if you want me to split them in two

maxlem-neuralium and others added 12 commits May 10, 2021 10:39
The eol character is now configurable
The user can now actuvate echo feedback to see what he is typing
I also removed a duplication in two run() overload, while fixing what I think was a mistake by enforcing the use of the provided argument Stream& even if the ctor one is not null.
Lastly, I warn the user if \n is configured but \r is detected (only in user friendly mode)
All the past code should work as before (no API change required)
I use
-D SIMPLEFOC_SAMD_DEBUG_SERIAL=SERIAL_PORT_MONITOR
I renamed the enum member EXTERN to EXTERNAL. For consistency, I also
renmaed INTERN.
The solution is to define them based on the equivalent rp2040 constants
until I find a way to make it compatible, it is disabled when compling
for rp2040. Problem is that SPIClass and SPISettings are not defined,
SPI seems to work somewhat differently in rp2040
@askuric askuric requested a review from runger1101001 May 13, 2021 08:04
@askuric
Copy link
Member

askuric commented May 13, 2021

No need, the other part of the code is merged already.

@runger1101001 are you ok with this serial modification?
Should we leave the Serial class usage by default?

@runger1101001
Copy link
Member

Basically the SAMD stuff is fine, but there are a lot of changes in Commander that I am uncertain about... I think unfortunately you guys were working on that in parallel.
I'd love to include the SAMD changes though, that makes total sense. I already did the Pico code's debug output in the same way...

At this point the way I would do it is to merge the changes over manually (locally), so that I can exclude the commander changes. I am more than happy to do this, but if you @maxlem would prefer to send another pull request excluding the commander changes, then GitHub would track that correctly. If I do it locally it will look like the change came from me...

Copy link
Member

@runger1101001 runger1101001 left a comment

Choose a reason for hiding this comment

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

Basically perfect, except we have parallel changes in Commander.cpp which can't be merged. Without the commander changes I would merge it!

@maxlem
Copy link
Contributor Author

maxlem commented May 18, 2021

Well, no more conflicts, but it's a mess.

I simply forgot to create a branch when I started to work on ADC-DMA stuff, it was a different feature.

In a perfect world, I would scrap all this and start a new fork, cherry-picking all the changes I really want to make it into dev

I'm so sorry :(

Tell me what you'd like I'll do it.

We cand always leave the ADC/LowSide stuff as it is since we're in a dev branch, but while the ADC (current_sense/hardware_specific/samd21_mcu.h) class is in a usable state, the LowSideCurrentSense.h is plain wrong, returns volts instead of amps right now and I know @askuric is working on its own

@runger1101001
Copy link
Member

Wow, now it says 51 files changed 😱

So I checked it out locally and applied it to the clean dev branch, and it changes only 10 files, and only the SAMD-related stuff. It is a FF merge.

So I will merge it. It can be reversed if it turns out different online than when I do it locally...

@runger1101001 runger1101001 merged commit e1f4292 into simplefoc:dev May 18, 2021
@runger1101001
Copy link
Member

Perfect, it worked out on GitHub just the same way as locally.
Supernice! I will try to test it out in the next few days!

Thank you so much, @maxlem for your hard work!

If I make make a suggestion, please rename your current "dev" branch (e.g. "dev-old") and create a new "dev" branch from the current simplefoc:dev branch. In this way your (future) changes will be easy to include.

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