kbembedded / Flipper-Zero-Game-Boy-Pokemon-Trading

Pokemon trade from Flipper Zero to Game Boy for Generation I & II games (Pokemon Red, Blue, Yellow, Gold, Silver, and Crystal)
MIT License
322 stars 18 forks source link

EXT interface board detection can falsly detect MALVEKE boards when a non-MALVEKE board is plugged in #25

Closed kbembedded closed 5 months ago

kbembedded commented 9 months ago

Describe the bug.

When implementing and testing the check everything seemed to work; however, I've recently found that when a non-MALVEKE board is plugged in at the start of the trade tool application, it is incorrectly detected as being present. I missed adding a delay between setting the pin states and reading them back. This has already been fixed and will be included in the 2.0 release.

WORKAROUND: If using a non-MALVEKE board to connect to a Game Boy to trade, be sure to select the pinout before entering the trade window, using the Original pinout will work for most non-MALVEKE boards.

Reproduction

N/A

Target Framework

No response

Logs

No response

Anything else?

No response

EstebanFuentealba commented 9 months ago

I believe the definitive solution will be to implement the Expansion Module Protocol.

kbembedded commented 9 months ago

Yeah, like I said on Discord, now that its standardized and has firmware functions, its far less expensive to implement something like it in-app.

If you want to add support for it in either the trade tool (or https://github.com/kbembedded/flipper-gblink even) then that would be awesome. The hard part will be getting everyone to update their boards and in such a way to detect which MALVEKE PCB rev to pick up the right pinouts. From my point of view though, it now introduces a 3rd or 4th MALVEKE variant that needs to be checked (MALVEKE boards with the MALVEKE EXT pinout and the Original EXT pinout, both with and without expansion module support in the ESP32).

Keeping the rudimentary MALVEKE checks in place as well as looking at the expansion module protocol ID, would be able to ID updated and non-updated boards that implement the expansion module protocol.

EstebanFuentealba commented 9 months ago

But the only variant is the MALVEKE pinout (PCB MALVEKE 2.5) board, which has the connected pins that freeze the flipper. PCB MALVEKE 2.5.1 and above will work with the ORIGINAL configuration, just like all the other connectors that already coexist.

kbembedded commented 9 months ago

Yes. My point is that we would want to identify PCBs using the MALVEKE EXT pinout. Which all of those are already in the hands of users, and in order to implement the expansion module protocol to ID it with the Trade Tool, those would all need to be updated. Not everyone will update their ESP32 to support the expansion module protocol, so now there would be need to detect the older MALVEKE board both with and without the update.

It cannot be assumed that if a board fails to ID over the expansion protocol that is is an older MALVEKE board because there are a handful of other EXT modules that would never be able to support the expansion protocol.

If you end up rolling out the expansion module protocol for MALVEKE's ESP32 and implement it in the trade tool, or gblink directly, that would be great. But IMO, it would need to be able to detect the two different mechanisms for the three different potential types; MALVEKE EXT pinout boards with and without expansion protocol support, and MALVEKE boards with Original pinout that all support the expansion module protocol.

If you want to completely drop the existing detection and enforce the expansion module protocol and otherwise fallback to Original pinout, then that is your call.

EstebanFuentealba commented 9 months ago

Another idea I have is to remove the automatic detection of the board and simply ask at the start of the app which connection is being used:

With the option to change it later as it is now. Perhaps it's an extra step, but I'm aiming for the simplest solution possible.

kbembedded commented 9 months ago

That is also a possibility. My goal was ease of use not necessarily ease of implementation. If you want to go that route I can certainly rip out the detection and add in needing to manually select the pinout and just defaulting to Original since that is the most prolific.

kbembedded commented 6 months ago

Fixed in #24