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

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

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

Arribe
Copy link
Collaborator

@Arribe Arribe commented Apr 2, 2024

Description

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

Closes #51

Dependencies

  • List any dependencies that are required for this change, including a full list of libraries required, especially if it is a new or otherwise unused library in the OpenHornet software.

Type of change

  • New software module (new software module for slave)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update outside of the automatically-generated Doxygen documentation.

Checklist:

  • My code follows the style guidelines of this project
  • I have complied with the software manual for this project
  • I have performed a self-review of my own code
  • I have commented my code fully with Doxygen compatible comments, particularly in hard-to-understand areas
  • I have made corresponding changes to non-Doxygen generated documentation
  • I have ran Doxygen locally, and it builds the docs successfully
  • My changes generate no errors on compile in Arduino IDE
  • My changes generate no new warnings on compile in Arduino IDE
  • Any dependent changes have been merged and published in downstream modules
  • (For sketches only) This sketch complies with OH-INTERCONNECT v10
  • If this sketch requires additional libraries, I have added it as a sub-module per the Arduino Libraries section of the Software Manual.

How Has This Been Tested?

  • I have tested the sketch in-circuit in DCS with DCS-BIOS and outputs (displays, LEDs, etc.) function as expected.
  • I have tested the sketch in-circuit in DCS with DCS-BIOS and HID inputs (switches, pots, etc.) function as expected, with switches moving the correct direction.
  • I have tested the sketch in-circuit in DCS with DCS-BIOS and any logic in the sketch has been tested and functions as expected.
  • This code has not yet been tested in-circuit.
  • This code has not yet been tested in DCS-BIOS.

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

  • Firmware version:
  • Hardware: OH 0.2.0
  • Toolchain:
  • SDK: DCS Bios 0.3.9

@kbastronomics
Copy link
Collaborator

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
Copy link
Collaborator Author

Arribe commented Apr 2, 2024

  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
Copy link
Collaborator

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
Copy link
Owner

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
Copy link
Collaborator Author

Arribe commented Apr 2, 2024

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
Copy link
Collaborator

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
Copy link
Owner

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

@Arribe
Copy link
Collaborator Author

Arribe commented Apr 4, 2024

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.

@jrsteensen jrsteensen added Type: Enhancement New feature or request. Category: Embedded This issue affects a specific embedded microcontroller software (i.e. an Arduino Sketch). P2 High Priority: This is the defect or issue which should be resolved before the release is made. Thes S1 Critical Severity: A defect that completely hampers or blocks testing of the product/ feature is a c labels Apr 6, 2024
@jrsteensen jrsteensen added this to the v0.1.0 milestone Apr 6, 2024
@jrsteensen jrsteensen linked an issue Apr 6, 2024 that may be closed by this pull request
16 tasks
Copy link
Owner

@jrsteensen jrsteensen left a comment

Choose a reason for hiding this comment

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

However we do it, unfortunately we will need it to pass CI, otherwise the entire repo will fail every CI check moving forward. CI is there to protect us and give us a little extra confidence in our software.

Failure Log:

4A9A1-THROTTLE_CONTROLLER.ino.cpp
MT6835.cpp
/home/runner/work/OpenHornet-Software/OpenHornet-Software/embedded/OH4_Left_Console/4A9A1-THROTTLE_CONTROLLER/build/4A9A1-THROTTLE_CONTROLLER.ino.cpp:93:10: fatal error: Wire.h: No such file or directory
   93 | #include <Wire.h>
      |          ^~~~~~~~
compilation terminated.
make[1]: *** [/home/runner/work/OpenHornet-Software/OpenHornet-Software/include/makeEspArduino/makeEspArduino.mk:308: build/4A9A1-THROTTLE_CONTROLLER.ino.cpp.o] Error 1
make[1]: *** Waiting for unfinished jobs....
In file included from /home/runner/work/OpenHornet-Software/OpenHornet-Software/libraries/Arduino-FOC-drivers/src/encoders/mt6835/MT6835.cpp:2:
/home/runner/work/OpenHornet-Software/OpenHornet-Software/libraries/Arduino-FOC-drivers/src/encoders/mt6835/./MT6835.h:8:10: fatal error: SPI.h: No such file or directory
    8 | #include "SPI.h"
      |          ^~~~~~~
compilation terminated.
make[1]: *** [/home/runner/work/OpenHornet-Software/OpenHornet-Software/include/makeEspArduino/makeEspArduino.mk:307: build/MT6835.cpp.o] Error 1
make[1]: Leaving directory '/home/runner/work/OpenHornet-Software/OpenHornet-Software/embedded/OH4_Left_Console/4A9A1-THROTTLE_CONTROLLER'
make: *** [Makefile:11: OH4_Left_Console/4A9A1-THROTTLE_CONTROLLER/] Error 2
Error: Process completed with exit code 2.

@Arribe
Copy link
Collaborator Author

Arribe commented Apr 7, 2024

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.

CI still doesn't work.
@Arribe
Copy link
Collaborator Author

Arribe commented Apr 20, 2024

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

@Arribe Arribe closed this Apr 20, 2024
@Arribe Arribe reopened this Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Embedded This issue affects a specific embedded microcontroller software (i.e. an Arduino Sketch). P2 High Priority: This is the defect or issue which should be resolved before the release is made. Thes S1 Critical Severity: A defect that completely hampers or blocks testing of the product/ feature is a c Type: Enhancement New feature or request.
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

[NEW SKETCH]: 4A9A1-THROTTLE_CONTROLLER.ino
3 participants