Skip to content

Simplification of implementation for new position sensors and current senses: generic sensor classes#145

Merged
askuric merged 2 commits intodevfrom
FEAT_generic_sensors
Dec 20, 2021
Merged

Simplification of implementation for new position sensors and current senses: generic sensor classes#145
askuric merged 2 commits intodevfrom
FEAT_generic_sensors

Conversation

@askuric
Copy link
Member

@askuric askuric commented Dec 17, 2021

A new approach for easier support of different sensors in the SimpleFOC.

I've created generic implementations for the position sensor (GenericSensor) and current sense (GenericCurrentSense).

Generic position sensor

GenericSensor requires the user to provide the sensor.readCallback function pointer which returns the float angle reading in between 0 and 2PI. Everything else is handled by the GenericSensor class. Additionally the user can provide the initialisation routine also sensor.initCallback which if provided is called in the sensor.init(). There are two ways of providing the functions to the GenericSensor

float readMySensor(){
 // read my sensor
 // return the angle value in radians in between 0 and 2PI
 return ...;
}
void initMySensor(){
  // do the init
}


// Provide the callbacks in the constructor
GenericSensor sensor = GenericSensor(readMySensor, initMySensor);

or

// empty constructor 
GenericSensor sensor = GenericSensor();

void loop(){
...
  // assign the callbacks directly
  sensor.readCallback = readMySensor;
  sensor.initCallback = initMySensor;
  sensor.init();
....
}

Generic current sense

GenericCurrentSense requires the user to provide the current_sense.readCallback function pointer which returns the PhaseCurrent_s stucture with current readings in Ampers. Everything else is handled by the GenericCurrentSense class. Additionally the user can provide the initialisation routine also current_sense.initCallback which if provided is called in the current_sense.init(). There are two ways of providing the functions to the GenericCurrentSense

float readMyCurrentSense(){
 // read my sensor
 // return the value in Ampers
PhaseCurrent_s c;
c.a = ..
c.b = ..
c.c = ...
 return c;
}
void initMyCurrentSense(){
  // do the init
}


// Provide the callbacks in the constructor
GenericCurrentSense current_sense = GenericCurrentSense(readMyCurrentSense, initMyCurrentSense);

or

// empty constructor 
GenericCurrentSense current_sense = GenericCurrentSense();

void loop(){
...
  // assign the callbacks directly
  current_sense.readCallback = readMyCurrentSense;
  current_sense.initCallback = initMyCurrentSense;
  current_sense.init();
....
}

I hope this will speed up and simplify the process of supporting the new sensors!

@askuric askuric changed the title Feat generic sensors Simplification of implementation for new position sensors and current senses: generic sensor classes Dec 17, 2021
@runger1101001
Copy link
Member

runger1101001 commented Dec 17, 2021

I like the generic sensor, this could be simpler for users to understand than the API based on subclassing the Sensor class...

I still have to think about the generic current sense. I guess the same applies here, but it is more tricky because of all the issues we discussed in the last call...

I've been working on that as we discussed, but I'm not yet ready to commit anything.

@askuric
Copy link
Member Author

askuric commented Dec 17, 2021

Yeah, you are right. This add-on is going to simplify both in my opinion. And also we will be able to support low-side current sensing easier here I think. Because the users can find the code that works for them then just plug it to the simplefoc using this.

Since it follows the same convention as the Inline and LowSide for the moment I'd merge this as is. To provide the users the way to test and try out. Maybe also leave the current sense as is for v2.2.1. Then do a proper restructure in v2.3.0 for example.
What do you think abou that?

@runger1101001
Copy link
Member

runger1101001 commented Dec 17, 2021 •
edited
Loading

Regarding the release schedule, I agree with that, because there are other fixes on the dev branch that people would want in the meantime.

Regarding plugging the current sense, I thought about it a bit, and I think this would remain a "pro user" thing. This is because even with the simplified callback based API for the current-sense the user will still have to know which timers the SimpleFOC configured in the driver in order to set up the DMA/events/interrupts correctly.

It is true that this might help users in the meantime, since they will maybe "know" the timers used for their board and pin configuration, and could just hard-code it. But at the moment there is no API to query the timers from the driver so they could not make a general solution.

On the other hand, this means it would lead to a bunch of timer-specific, pin-specific, board-specific code, which might really confuse things when people start sharing it in the forum...

So I'm not sure on the current sense.

@askuric
Copy link
Member Author

askuric commented Dec 20, 2021

Ok, so current sense will, even with this change remain pro user thing. Exactly, they do need to know how sensors work in order to use these generic sensors . User will need to either figure it out or use someone else's code to do it. But the good thing here is that the someone else's code does not have to be simplefoc related.
Maybe someone wants to add an external ADC and use with some custom communication or just found a code that works and they would like to integrate with the simplefoc.
Since there is an universe of possible combintations of features that we would (will) have to support to make the true current sense abstraction for everyone, this is a simple way for the community to contribute without needing to go to the depths of our API.

Now, is it a good way, maybe not. But at the moment it is the simplest one.

And in terms of the confusion abut the timers and low level api in the community, I think this is actually good, because some good solutions will come out and that will maybe help us make right choices in future about the most natural way to support hardware.

So I am still pretty much confident that this can be merged, now if you really think that this is a bad idea, then I'll split the code and we will talk about this thing some other time. :D

@runger1101001
Copy link
Member

runger1101001 commented Dec 20, 2021

I say go for it! :-)

@askuric askuric merged commit 82178dc into dev Dec 20, 2021
@askuric askuric deleted the FEAT_generic_sensors branch December 22, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@askuric @runger1101001