Logitech G25 and G27 Shifter Support #19
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This more accurately describes what the constructor is *asking*, if not how the code actually works.
Although all current peripherals use active-high detection, that may not be the case in the future. Changing this now for future compatibility. The Logitech classes maintain their original arguments, as we know they are active-high. Because the added argument includes a default, this does not technically break the public API.
Changes out the direct access to 'currentGear' and the 'changed' flag with a buffer function that handles that for us. Much cleaner and safer. Not sure what I was thinking the first time around...
This allows for limiting the shifter range, say if you want to build a 5-speed analog shifter. More importantly it allows us to put the shifter in reverse without having to define a pin to read the reverse signal from.
In implementing the G25 shifter I was running into synchronicity issues since each 'update()' function polled the connection state independently. In some updates the buttons would be connected while the shifter wasn't, in other updates it was the other way around. Moving the detector behavior to the base class makes it so that the detector is only polled once per update(), and it's *always* polled per update(). Using a pointer here also allows future classes to take advantage of polymorphism for more complicated 'detection' systems.
With the implementation of connection in the base class, caching this value allows us to clear it on disconnect. It also reinforces the idiom that peripheral state is only changed on 'update()'.
Extends the LogitechShifter class, and adds support for reading the shift registers connected to the shifter's additional buttons.
Extends the LogitechShifterG27 class, and adds support for the sequential shifting feature
There seem to be a number of users confused about what the "Driving Force" shifter is. These aliases cover users who are using the shifter that came with their wheel and know their wheel model only.
And puts them in their own folder, as the library now supports multiple different shifters (although they're all Logitech at the moment...)
Mostly because the CS pin pull-up is absolutely necessary for the 'reverse' signal to work on the Driving Force shifters.
This should be const qualified as it was before. The class state should change on 'update()' and the detector state should change on 'poll()'. This function should never change the class state.
This works fine on my shifter with the DrivingForce calibration, but is more accurate with its own.
Convenience function for creating objects that are compatible with the Sim Racing Shields (https://github.com/dmadison/Sim-Racing-Shields) without needing to look up or remember the pinout.
These were well-intentioned and easy for the end user, but they aren't flexible. The G25/G27 shifters require different pin arguments and do not work on the v1 shield. These macros have been superseded by the `CreateShieldObject` template function.
Matches the layout of the G25 and G27 examples, and shows the user that this feature is available even if it requires extra hardware.
Reading back these descriptions and it sounds like the device is holding the pin state, rather than the pin state being driven by the micro (read: library). Rewriting to be clearer.
This lets you read the Driving Force shifter with the G27 object. As the detection pins are the same, the two shifters cannot be simply distinguished by the microcontroller.
The G27 shifter, although it looks similar to the G25, does *not* have the same pinout (pins 1 and 7 are swapped). This fixes all G27 pinout references throughout the library.
The pinout difference between the two is significant enough to necessitate separate documentation. The G27 documentation page is based on notes from disassembling my own G27 shifter.
These default engagement / release points were already set as static members, but I forgot to add them as defaults to the calibration function.
In testing the G25 calibration worked fine for my G27 shifter, but doing my due diligence to set the shift points based on the actual hardware.
Although I think the detector being a part of the Peripheral base class is wise, I'm not a fan of passing the detector pointer through the constructor. If a derived class passes its own detector and a later derived class wants to change that detector for whatever reason, it's blocked. Changing to a protected function makes more sense. This also in effect removes arbitrary detection support for the generic base classes (Shifter, Pedals, Handbrake, etc.). I have no evidence that that feature was ever used by an end-user, and with the way the library is structured that is better implemented as a custom derived class for their specific device. It can always be added back in later if necessary (via a second constructor taking a pin and declaring an object on the heap, most likely).
The resistor is not strictly necessary, although it does make a lot of sense on the G25 to match brightness between the two indicator LEDs.
This is arbitrary, but it's important to get this change in there before the API is locked for the major release. Originally, I figured that resistors were required for both pins (pull-down for the detect, series for the LED). In which case, it's more likely that users would want the detect feature (disabling inputs on disconnect) over the LED feature (...blinky). After some testing I've determined that even though the LED should probably have a 100 Ohm resistor, it will survive without it. That means it's more likely for users to set it, in which case it should take precedence over the detect pin. In addition, the LED pin is shared by the data input of the EEPROM chip. Although the library does not currently interface with the EEPROM, if it ever does that pin will become required. In which case, again, the detect pin would end up at the end of the list so it can have a default argument.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds library support for the Logitech G25 and Logitech G27 shifters, including support their button panels and documentation about their wiring and internal connections.
Logitech G27
The Logitech G27 shifter has an H-pattern shifter, 8 action buttons, and a directional pad.
Implemented with the
LogitechShifterG27class, inheriting fromLogitechShifterand adding support for the button panel. Includes alogitech_shifter_g27documentation page with information about the shifter's internals and connections.Logitech G25
The Logitech G25 shifter has an H-pattern shifter, 8 action buttons, a directional pad, and the ability to transform into a sequential shifter.
Implemented with the
LogitechShifterG25class, inheriting fromLogitechShifterG27and adding support for sequential shifting. Includes alogitech_shifter_g25documentation page with information about the shifter's internals and connections.In order to support the new shifters, this PR makes a few miscelaneous changes to how the library works.
Peripheral Changes
updateState(bool connected)function. Classes should no longer define their own version ofupdate(). (breaking change)DeviceConnection Changes
DeviceConnectiondetection is now handled in the basePeripheralclass. Derived classes must pass a pointer to a detection object and handle its lifetime themselves. (breaking change)Shifter Class Changes:
Gearalias for gear numbersShifterbaseAnalogShifter(up to 6)LogitechShifterG923,LogitechShifterG920,LogitechShifterG29)Shield Support Changes:
PEDAL_SHIELD_V1_PINS,SHIFTER_SHIELD_V1_PINS), which would cause uncaught errors if used across the different supported classes. (breaking change)CreateShieldObject<class T, uint8_t version>()template function, which protects against creating incompatible objects by throwing linker errors if attempting to use the wrong major version of the PCB.Misc Changes:
This PR contains a number of breaking interface changes, and will require a new major library version when merged.