Skip to content

Adding a PositionAPI#149

Open
jalinei wants to merge 1 commit into
owntech-foundation:mainfrom
jalinei:positionAPI
Open

Adding a PositionAPI#149
jalinei wants to merge 1 commit into
owntech-foundation:mainfrom
jalinei:positionAPI

Conversation

@jalinei
Copy link
Copy Markdown
Contributor

@jalinei jalinei commented Mar 23, 2026

So far we add a sensorAPI oriented for ADC measurement only, but it was not covering the external position sensors that can be attached to an ownverter for instance.

In this PR, we address that issue so that one could use

  • Hall effect based sensors
  • Incremental encoder AB index
  • Sin/cos sensors

With the same generic calls.

Specific dts fragments and bindings have been added to cover the required hardware informations.

Shields overlays now encompass timer definitions for AB index sensors. (Legacy implementation relied on spin.dts to define the pinout)

My proposal is to test against the FOC example modified to use that API. From there, the same example should be usable with one of these 3 sensors in an agnostic way. (except choosing the right sensor and providing motor info from app.overlay).

Signed-off-by: Jean Alinei <jean.alinei@owntech.org>
@jalinei jalinei requested a review from luizvilla March 23, 2026 13:25
@jalinei
Copy link
Copy Markdown
Contributor Author

jalinei commented Mar 23, 2026

Matching example to try this new API :

https://github.com/jalinei/examples/tree/test_multi_sensor

@luizvilla luizvilla changed the title Adding a proper PositionAPI Adding a PositionAPI Apr 22, 2026
@luizvilla
Copy link
Copy Markdown
Member

luizvilla commented Apr 22, 2026

Review of the PositionAPI

Some changes were made to the API to accomodate needs for the encoder. (described below)

The code that was tested is in the branch: https://github.com/luizvilla/Core/tree/Regression_testing_position_API


Encoder testing

On open-loop control, here is the result from the encoder:

The open loop control is available by using the 'l' key and the 'p'. You can then raise the 'vq_ol' reference and the 'omega_ol'

open-loop_test_encoder _Open-loop test of the encoder_

The sub-plot on the bottom shows two triangles, a red and a gray one.

  • Red triangle is the measured angle from the PositionAPI
  • Gray triangle is the angle generated automatically via the open-loop control loop.

The triangles have the same shape, and a phase shift. This is expected behaviour as the code is creating the phase.
For me this validates that the angle measurement is correct.

@luizvilla
Copy link
Copy Markdown
Member

Hall testing did not work with this code. I'm reverting everything and verifying again.

@luizvilla
Copy link
Copy Markdown
Member

luizvilla commented Apr 22, 2026

Steps

The example does not compile out of the box due to an extra file:

  • app copy.overlay

It has to be removed.

For now the original Hall sensor code does not work.

I looked up the signals on the board and the hall sequence. I have B working and A/C always at zero.
On the oscilloscope side, the signals are fine as per the two images below.
scope_15
_A and B seen with two probes on the SPIN _

scope_18 _A and C seen with two probes on the SPIN_

I've patched the Position API to see the raw signals during a full revolution:

A = 0, B=1 (oscillates) and C = 0.

There is a problem with signals A and C on my implementation.

I've swapped the gpios on the ownverter overlay

		hall: hall-sensor {
			compatible = "shield-position-hall";
			position-sensor-name = "HALL";
			motor = <&default_motor>;
			direction-sign = <1>;
			electrical-offset = <0x00000000>;
			hall-a-gpios = <&spin_header 7 GPIO_ACTIVE_HIGH>;
			hall-b-gpios = <&spin_header 9 GPIO_ACTIVE_HIGH>;
			hall-c-gpios = <&spin_header 49 GPIO_ACTIVE_HIGH>;
			hall-sector-table = <5 1 0 3 4 2>;
			hall-interpolation = "NONE";
			status = "okay";
		};

with :

A B C result
7 9 49 0 - 1 - 0
9 7 49 1 - 0 - 0
7 49 9 0 - 0 - 1

PINS 7 and 49 are not working as of now.

I found the specific part of the API that handles the pins:

	uint8_t hall_a = spin.gpio.readPin(hall_config->hall_a_pin);
	uint8_t hall_b = spin.gpio.readPin(hall_config->hall_b_pin);
	uint8_t hall_c = spin.gpio.readPin(hall_config->hall_c_pin);

...

	spin.gpio.configurePin(sensor_prop->configuration.hall.hall_a_pin, INPUT);
	spin.gpio.configurePin(sensor_prop->configuration.hall.hall_b_pin, INPUT);
	spin.gpio.configurePin(sensor_prop->configuration.hall.hall_c_pin, INPUT);


Solved by changing the way the data is parsed from the ownverter overlay.

Currently it does

#define POSITION_HALL_A(node_id) \
	DT_GPIO_PIN_BY_IDX(node_id, hall_a_gpios, 0)

#define POSITION_HALL_B(node_id) \
	DT_GPIO_PIN_BY_IDX(node_id, hall_b_gpios, 0)

#define POSITION_HALL_C(node_id) \
	DT_GPIO_PIN_BY_IDX(node_id, hall_c_gpios, 0)

Which parses the spin_header.dtsi

		           <7 0 &gpioc 6 0>,	/*  7 */
		           <9 0 &gpioc 7 0>,	/*  9 */
                            ...
		           <49 0 &gpiod 2 0>,	/* 49 */

giving to 7 = 6, 9 = 7 and 49 = 2

With the fix, it will no longer drop the &gpioa, &gpiob, &gpioc, &gpiod.

define POSITION_GPIO_PORT(node_id, prop) \
	(DT_SAME_NODE(DT_GPIO_CTLR_BY_IDX(node_id, prop, 0), DT_NODELABEL(gpioa)) ? PA : \
	 DT_SAME_NODE(DT_GPIO_CTLR_BY_IDX(node_id, prop, 0), DT_NODELABEL(gpiob)) ? PB : \
	 DT_SAME_NODE(DT_GPIO_CTLR_BY_IDX(node_id, prop, 0), DT_NODELABEL(gpioc)) ? PC : \
	 DT_SAME_NODE(DT_GPIO_CTLR_BY_IDX(node_id, prop, 0), DT_NODELABEL(gpiod)) ? PD : 0)

#define POSITION_GPIO_ENCODE(node_id, prop) \
	(POSITION_GPIO_PORT(node_id, prop) | DT_GPIO_PIN_BY_IDX(node_id, prop, 0))

@luizvilla
Copy link
Copy Markdown
Member

luizvilla commented Apr 22, 2026


Sequence matching (for later)
Since I changed the sequence, I have to update it back again.

I will test the positions to verify it works as follows (R = Red cable, W = white cable, B = black cable)

Position V1 V2 V3 PASS/FAIL
1 R W B FAIL
2 R B W FAIL
3 W R B FAIL
4 W B R FAIL (short)
5 B W R FAIL
6 B R W FAIL

We ran a series of tests with @jalinei using the original code.
It worked with the original code.

@luizvilla
Copy link
Copy Markdown
Member

Current state of advancement

I made a series of commits on my PositionAPI_corrected branch.

I have added:

  • The correction for the HALL effect sensor described above
  • Three extra parameters of the ABZ, namely (index-present, index-polarity, index-configuration)
		abz: incremental-encoder {
			compatible = "shield-position-abz";
			position-sensor-name = "ABZ";
			motor = <&default_motor>;
			direction-sign = <1>;
			electrical-offset = <0x00000000>;
			timer = <&timers3>;
			counts-per-revolution = <1024>;
			index-present = <1>;
			index-polarity = "INVERTED";
			index-configuration = "A_HIGH_B_HIGH";
			status = "okay";
		};
  • An exposure of these parameters via the PositionAPI linking all the way down to the timerHAL.

  • These updates on the encoder side yielded the results shown above, with a triangle shaped curve.

@luizvilla
Copy link
Copy Markdown
Member

luizvilla commented Apr 23, 2026

@jalinei

I have created a library of examples for the OwnVerter, several of which uses the position API.
They all compile in my machine.
They are based on the existing example, but split it into 9 new ones.
They are here:

https://github.com/luizvilla/examples/tree/new_OWT_examples

They require using the positionAPI_corrected branch from my core:

https://github.com/luizvilla/Core/tree/positionAPI_corrected

Testing procedure:

  • checkout my branch positionAPI_corrected
  • update the platformio.ini line
owntech_examples = https://github.com/luizvilla/examples.git#new_OWT_examples
  • choose an example from the alien drop down
image

have fun!

@luizvilla
Copy link
Copy Markdown
Member

With all the previous messages you have:

  • all the work I've done around the positionAPI consolidated into a clean commit logic
  • all the testing work you have done with the overall example broken down into simpler examples which can be tested and used by new users

Hope it helps

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.

2 participants