jrsteensen / OpenHornet-Software

Repo for all the software that brings OpenHornet to life!
Other
14 stars 8 forks source link

[NEW SKETCH]: 4A9A1-THROTTLE_CONTROLLER.ino #101

Open Arribe opened 3 months ago

Arribe commented 3 months ago

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.

Closes #51

Dependencies

Type of change

Checklist:

How Has This Been Tested?

Description of Testing

Generally the throttle is working as a joystick in Windows and DCS when controls are mapped, like any other commercial throttle.

The solenoids are not turning off, and are always on. Even when a simple test code of always setting the signal pin to LOW.

Test Configuration

kbastronomics commented 3 months ago

This is including a 3rd part product. license must be verified it is compatible with OpenHorner. SimpleFOC is using the MT license there' is no attribution for the SimpleFOC per SimpleFOC requriemenet which violate thier license https://github.com/simplefoc/Arduino-FOC?tab=MIT-1-ov-file#readme must including citation as mentioned if we use their code.

also the actual code for the MT6935 must be extracted, this is including the entire driver based of SimpleFOC into OH by these includes

include "SimpleFOC.h"

include "SimpleFOCDrivers.h"

include "encoders/mt6835/MagneticSensorMT6835.h"

this basically pulls in all of SimpleFOC into the throttle code then doesn't use it.

Joystick.setRxAxisRange(0, 2048); // Outboard Throttle Arm Joystick.setRyAxisRange(0, 2048); // Inboard Throttle Arm

temp = outboardThrottle.readRawAngle21(); // read outboard hall sensor Joystick.setRxAxis(map(temp, 0, 707444, 0, 2048)); we read the 21bit value then map it to 11 bits???

why are we using 11 bits of the MT6835 when its a 21bit HAL sensor (internally) that's a big loss of resolution of the throttles.

code could be simpllied using and change to 16bit resolution Joystick.setRxAxis(map(outboardThrottle.readRawAngle21(), 0, 707444, 0, 65535)); //

Can we please stop using arrays when basic calls would be clearer. code is overly complicated to read for the average user. have no idea what array element does what. refencing by element number, please use defines instead of index values

we're defining REVERSE_EXT_LTS to handing a miswriting by the coder but don't do this for all switches either none should be or all should be done.

missing documentation on protocol used to talk to the inner grip PCB. Wire.requestFrom(49, 17); // what error checking is done?

Arribe commented 3 months ago
  1. This is including a 3rd part product... A: Added comment with link to Simple FOC copyright notice. If / when this PR is merged the Open Hornet repository will have a copy of the Simple FOC library with all files as a git-submodule.

  2. also the actual code for the MT6935 must be extracted... A: Added as a todo comment.

  3. Joystick.setRxAxisRange(0, 2048); // Outboard Throttle Arm Joystick.setRyAxisRange(0, 2048); // Inboard Throttle Arm

temp = outboardThrottle.readRawAngle21(); // read outboard hall sensor Joystick.setRxAxis(map(temp, 0, 707444, 0, 2048));

we read the 21bit value then map it to 11 bits???

why are we using 11 bits of the MT6835 when its a 21bit HAL sensor (internally) that's a big loss of resolution of the throttles.

A: That was from lots of testing...but revisited. The Arduino map function was rolling over its mapped output values at 2,800. Don't know why since the map function was using longs. It should have more than enough headroom for these numbers. Re-wrote the map function for the Hall Sensors to use unsigned long long. After that update the map function works correctly with the max output of 65,535.

  1. code could be simpllied using and change to 16bit resolution Joystick.setRxAxis(map(outboardThrottle.readRawAngle21(), 0, 707444, 0, 65535)); // A: Spent so much time tweaking and adjusting the map function's input min max based on reading the raw values. Don't know if what I see will be the same for all users. They may need to send the raw reads to the serial monitor so reading to a temp value to help facilitate. Added a TODO to remove if we find it's not needed, and the code can be simplified.

  2. Can we please stop using arrays when basic calls would be clearer. code is overly complicated to read for the average user. have no idea what array element does what. refencing by element number, please use defines instead of index values

A: Removed arrays, changed to just a series of reads and wire writes. The controller wire reads stayed in a while loop to facilitate stepping through the inner grip values. Added defines for the index values based on inner grip control name. Refactored to a switch statement on index to use the defines for more explicit mapping from control name to Joystick button #.

  1. we're defining REVERSE_EXT_LTS to handing a miswriting by the coder but don't do this for all switches either none should be or all should be done.

A: Removed REVERSE_EXT_LTS logic.

  1. missing documentation on protocol used to talk to the inner grip PCB. Wire.requestFrom(49, 17); // what error checking is done?

A: added additional comments. Error checking added to a TODO.

kbastronomics commented 3 months ago

Thanks for the updates in the end we do not want to include the entire simpleFOC library as part of OH we (I) need to extract only that part of it and make it stand alone. just navigating thier code is like following your own adventure

jrsteensen commented 3 months ago

I sort of envisioned us creating a library (that may be useful to other projects) just for these hall sensors. Maybe that would consist of the cut-down simpleFOC code? (See #8 )

Arribe commented 3 months ago

It would be nice to have our own small library for the hall sensors. But if I'm being honest, it is beyond me unless I start lifting code from Simple FOC.

I've spent a lot of time playing with the various raw reads, mapped values, and final joystick values getting passed to Windows. It is not at all straightforward to trouble shoot which step is causing the challenges I saw. I also see slight numeric differences in values, not only between the throttles, but also from test to test with the same throttle arm. It will be interesting to have others share their results on the hall sensors. My hunch is there are going to be differences in sensor values between users.

kbastronomics commented 3 months ago

I sort of envisioned us creating a library (that may be useful to other projects) just for these hall sensors. Maybe that would consist of the cut-down simpleFOC code? (See #8 )

Yes, that's been my intent, no so much extracting their code but understanding to make our own.

jrsteensen commented 3 months ago

Do we want to hold on this until that little lib is complete? What sort of ETA could we expect on it @kbastronomics?

Arribe commented 3 months ago

Up to the group on what to do with it. Can't get it to compile on the CI. Though likely most people will download the files, install the libraries and then compile locally for an upload to their controller.

Arribe commented 3 months ago

ESP32 is failing to compile even though the Wire and SPI libraries are part of the base https://github.com/espressif/arduino-esp32 repository. I've added notes to Bug #102 with some testing I've done.

Arribe commented 2 months ago

ESP32 doesn’t compile via the CI/CD. Closing so someone else can fix it.