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

Feat pwm spi #92

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open

Feat pwm spi #92

wants to merge 16 commits into from

Conversation

maxlem
Copy link
Contributor

@maxlem maxlem commented Jun 8, 2021

one widespread change: _readADCVoltagesLowSide(a,b,c)

Justification: only used in LowsideCurrentSense, we're always reading in pairs(triple) and it enables async (interrupt, dma) and concurrent (multiple ADC) implementations

the rest of potentially conflicting changes are guarded by the SAMD21_ASYNC tag

this MR brings in a MAJOR effort from my part to enable async ADC and SPI readings with the samd21

maxlem added 13 commits May 21, 2021 17:18
use of EVSYS to trigger ADC conversion at each TCC OVF
removed DMA code as I couldn't figure out how to combine both

Problem1: Currently I only get straight 1.71V +/- 0.01 no matter what happens
with the motor /control loop. Something is off with the timing
Problem2: all OVF signal triggers the ADC, which is configured in INPUTSCAN mode and iterate over the 3 channel at each event trigger. while events come from different channels, they all sink into the same USER channel and there is now way of correctly mapping e.g. TCC0 to ADC pinA
replaced a few const char* with PSTR()
new  _SAMD21_EVSYS_ define
@maxlem maxlem changed the base branch from master to dev June 8, 2021 17:58
@runger1101001
Copy link
Member

Awesome! Thanks so much for all this work!!

I'd love to just merge it, but I fear there are a couple of issues:

  • I can't comment on the current sensing side, I will leave that to @askuric who is working on the current sensing APIs, and will know much better whether the changes match with his ideas...

  • for the driver side, I see what you have done. I think it is good, but I think we need to "clean up" the API between the driver and the current sensing code, because most MCU architectures will have similar needs in terms of needing to know which timer units to connect with which ADC units. So it would be cool if we could extract a general API for this.

  • for the driver side, I get the feeling that the modifications as they stand imply certain pin configurations. The driver code currently works even if the TCC units are all different, and on SAMD21 even if some of them are TC units... so I think there is a hidden dependency there when using the current sensing, since the events are only activated on the TCC unit of phase A... I have to think about how to handle this. I guess it needs the clock sync you mentioned. I don't think it can work with TC units.

  • for the common SPI code - I have to look at it - I have to say I wasn't expecting this! on the one hand, this is getting very hardware and application specific... on the other hand, its so cool... I'm not sure if it would not be better if we could package it as part of a specific sensor implementation which we keep in the drivers repository, because it is a very specific thing... I think you might have to take us through it :-) and explain what's going on

  • for the sensor changes, this unfortunately comes at a bad time, where I've refactored the Sensor API in parallel to the work you did. So there is definately some work there to merge those two concepts.

@runger1101001
Copy link
Member

Another question, looking at it - will it support more than one motor? It seems the current sensing code is using a variable called instance - maybe I don't understand the API fully yet...

@maxlem
Copy link
Contributor Author

maxlem commented Jun 9, 2021

Hello!

Thanks for the review.

I tried to touch the driver side as little as possible. I agree it is not perfect as it is. I think it could work with TC units.

For the sensor API, I just created a new abstract base class which contains most of the common logic. Unfortunately, Arduino's SPIClass is pretty much sealed up so I had to create a new one. Maybe we could use templates and duck typing to create abstraction.

The only real reason for the "instance" thing is to please the current sensing API, which would not support more than one motro either.

To support a second motor, you would need a second ADC (samd, same), though. I repeat, the current code uses the ADC in ** input scan** mode. When it receives an event, it starts the scan process, and samples pins A,, B, C, then stop. this is all hardware.

What we "could* do is scan more than 3 pins (e.g. 6) on every signal, but it would becomes messy. better use interrupts and suffer.

@maxlem
Copy link
Contributor Author

maxlem commented Jun 10, 2021

so @runger1101001
You want me to move the SPI/Sensor stuff to the drivers repository?
The thing is, SPIClass is barely usable with other sercoms than sercom4 with the state of variant.cpp. I'm trying to make things happen on the Arduino side. but I hear only crickets.

For example, to use sercom5, you have to call
pinPeripheral(SPI5_MOSI, EPioType::PIO_SERCOM_ALT);
pinPeripheral(SPI5_MISO, EPioType::PIO_SERCOM);
pinPeripheral(SPI5_SCK, EPioType::PIO_SERCOM);
after any call to SPIClass::begin() to undo harm done by it.

Also, my class is nicely telling you if you're using a valid pin/pad assignement.

we could make the SPI type a template parameter (which defaults to SPIClass) and reproduce SPIClass API.

But if that's what you really prefer, I'll move it to drivers

@askuric
Copy link
Member

askuric commented Jun 12, 2021

Hey @maxlem,

First of all thanks for the great work. The depth you have gone into is impressive. 😄

Now, in terms of low side current sensing I cannot really test it at the moment, I do not have any samd21 chips with me. As soon as we can test it I would be in to merge it. Or even merge it before but I'll have to document everything properly so that the people know it has not been properly tested.

Now, in terms of SPI sensor, I'd move this to the drivers repository. It is far too much for this library. It is really cool what you have done, but simplefoc is really not the "advanced sensor driver" but the FOC driver. I know we have mixed a lot of stuff already in this repository but we are always trying to support as little as possible hardware specific code inside of the simplefoc.
This might change some time in future, but for now we are heavily leaning on the simplicity over optimality.
But I think you have done a great work and I am sure some people will greatly benefit of it!
So please do communicate it with Richard and I am sure you'll find a good way to integrate it into the drivers repo!

Now in terms of api changes.

  • coupling the three getters for three phase currents is a good idea. But _readADCVoltagesLowSide(a,b,c) is too constraining, we should at least communicate the pin numbers to the hardware specific code _readADCVoltagesLowSide(a,b,c, pinA, pinB, pinC)
  • We are definitely going to try to support more than one motor with low-side current sensing per mcu. I see that you've taken the inspiration from the ESP32 implementation, but that implementation is very basic and we will support at least one low side sensor per MCPWM for esp32. I know this will not maybe be the case for all mcu architectures but I'd definitely like the api to be able to support it.

At this point we are restructuring the library and we are going to allow for the communication in between driver code and current sensing code in a form of shared configuration structures or classes or something like that. We have still not decides 100%.
But this will change the driver and the current sense API for sure. I'll try to make a proposal and make a pull request this weekend so we can discuss it and I'd love to hear what your opinion as well.

@askuric
Copy link
Member

askuric commented Jun 13, 2021

Although, i have to say that i really like how you've added the hardware_specific folder to the senors also.
I was thinking that we might have to do that not just for magnetic sensors but for encoders too. There are many architectures with hardware counters that remove the need for interrupts.

Do you think it's a good idea to have all hardware specific code in the same place, or you think it would be better to leave the stuff as is. As is maybe makes more sense for sensors.

@runger1101001
Copy link
Member

Hey - I'll make myself available and find some time over the course of next week to integrate @maxlem 's code. If it is ok with everyone I would put all the sensor stuff into the drivers repository for now, and concentrate on getting the current sensing into the main repo.

I think we need to be careful not to introduce to many new changes in one release, or we'll get in a muddle, or have lots effort testing - or have lots of bugs.

And also I would focus on changes that "most users" would benefit from for the main repo (for now). And although I quite like it after writing all that code for it, SAMD21 isn't the architecture most used with SimpleFOC.

We made the drivers repo to have a place for all the often hardware specific code that is clearly useful and connected to SimpleFOC, but probably not used by most of the users. It's also so we can have different development cycles and levels of testing on these two code bases.

All this is of course 100% your call, Antun, but that's how I would see it.

And regarding your question, Antun, my opinion is that leaving the sensors as is for now is better. We can always come back and revisit that question next release...

@maxlem
Copy link
Contributor Author

maxlem commented Jun 14, 2021

All good points!

Personally, I don't like this _apiFunction() with global variables hidden in cpp files. We should use OO instead. If you fear for virtual table lookups, we can always use generic programming (templates) instead

I will move the SPI stuff away from this MR, I don't really mind. Still, I think my abstract MagneticSensor class is right on the money (except maybe the name). I think this class should be the base class for any sensor that has an absolute reading in some integer form.

@maxlem maxlem closed this Jun 14, 2021
@maxlem maxlem reopened this Jun 14, 2021
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.

3 participants