search

ducalex / retro-go

Retro emulation for the ODROID-GO and other ESP32 devices
GNU General Public License v2.0
489 stars 114 forks source link

Added support of a custom device Esp32s3-devkit-c #100

Closed Akrobate closed 5 months ago

Akrobate commented 5 months ago
ducalex commented 5 months ago

Can you split the docker changes to another PR? I think it's a good idea to add it to the project but I'd like to test it separately. Also currently I think it will break our Github Actions, but maybe I'm misunderstanding the changes...

The rest of the PR looks good but I'm about to cause a conflict and I would prefer if you fixed it yourself because you'll need to test if it still works on your board.

Though I did prepare a patch to save you time. Just undo rg_input.c and apply the following:

diff --git a/config.h b/config.h
index b4464c48..e2cefc5a 100644
--- a/config.h
+++ b/config.h
@@ -46,21 +46,26 @@

 // Input
-#define RG_GAMEPAD_DRIVER           7   // 1 = ODROID-GO, 2 = Serial, 3 = I2C, 7 = Like ODROID but all pull up pins
+#define RG_GAMEPAD_DRIVER           1   // 1 = ODROID-GO, 2 = Serial, 3 = I2C
 #define RG_GAMEPAD_HAS_MENU_BTN     1
 #define RG_GAMEPAD_HAS_OPTION_BTN   1
-// Note: Depending on the driver, the button map can be a bitmask, an index, or a GPIO.
-// Refer to rg_input.h to see all available RG_KEY_*
-#define RG_GAMEPAD_MAP {\
-    {RG_KEY_SELECT, RG_GPIO_GAMEPAD_SELECT},\
-    {RG_KEY_START,  RG_GPIO_GAMEPAD_START},\
-    {RG_KEY_MENU,   RG_GPIO_GAMEPAD_MENU},\
-    {RG_KEY_OPTION, RG_GPIO_GAMEPAD_OPTION},\
-    {RG_KEY_A,      RG_GPIO_GAMEPAD_A},\
-    {RG_KEY_B,      RG_GPIO_GAMEPAD_B},\
+#define RG_GAMEPAD_ADC1_MAP {\
+    {RG_KEY_UP,    ADC1_CHANNEL_5, ADC_ATTEN_DB_11, 3072, 4096},\
+    {RG_KEY_RIGHT, ADC1_CHANNEL_6, ADC_ATTEN_DB_11, 1024, 3072},\
+    {RG_KEY_DOWN,  ADC1_CHANNEL_5, ADC_ATTEN_DB_11, 3072, 4096},\
+    {RG_KEY_LEFT,  ADC1_CHANNEL_6, ADC_ATTEN_DB_11, 1024, 3072},\
+}
+#define RG_GAMEPAD_GPIO_MAP {\
+    {RG_KEY_SELECT, GPIO_NUM_16, GPIO_PULLUP_ONLY, 0},\
+    {RG_KEY_START,  GPIO_NUM_17, GPIO_PULLUP_ONLY, 0},\
+    {RG_KEY_MENU,   GPIO_NUM_18, GPIO_PULLUP_ONLY, 0},\
+    {RG_KEY_OPTION, GPIO_NUM_8,  GPIO_PULLUP_ONLY, 0},\
+    {RG_KEY_A,      GPIO_NUM_15, GPIO_PULLUP_ONLY, 0},\
+    {RG_KEY_B,      GPIO_NUM_5,  GPIO_PULLUP_ONLY, 0},\
 }

 // Battery
+#define RG_BATTERY_DRIVER           1
 #define RG_BATTERY_ADC_CHANNEL      ADC1_CHANNEL_3
 #define RG_BATTERY_CALC_PERCENT(raw) (((raw) * 2.f - 3500.f) / (4200.f - 3500.f) * 100.f)
 #define RG_BATTERY_CALC_VOLTAGE(raw) ((raw) * 2.f * 0.001f)
@@ -68,16 +73,6 @@
 // Status LED
 #define RG_GPIO_LED                 GPIO_NUM_38

-// Built-in gamepad
-#define RG_GPIO_GAMEPAD_X           ADC1_CHANNEL_6
-#define RG_GPIO_GAMEPAD_Y           ADC1_CHANNEL_5
-#define RG_GPIO_GAMEPAD_SELECT      GPIO_NUM_16
-#define RG_GPIO_GAMEPAD_START       GPIO_NUM_17
-#define RG_GPIO_GAMEPAD_A           GPIO_NUM_15
-#define RG_GPIO_GAMEPAD_B           GPIO_NUM_5
-#define RG_GPIO_GAMEPAD_MENU        GPIO_NUM_18
-#define RG_GPIO_GAMEPAD_OPTION      GPIO_NUM_8
-
 // SPI Display (back up working)
 #define RG_GPIO_LCD_MISO            GPIO_NUM_NC
 #define RG_GPIO_LCD_MOSI            GPIO_NUM_12
ducalex commented 5 months ago

Also I've just added photos for the other devices. If you'd like you could add one of yours!

Akrobate commented 5 months ago

Ok no problems, I'll put my docker modifications in an other PR. I haven't seen that it's used in your CI and my changes on the docker file will break the CI. I have two ideas to manage it properly. Will show you it later in an other PR =)

Ok for your changes will try to test it today and will update my branch.

Little suggestion in naming in RG_GAMEPAD_ADC1_MAP maybe you can remove ADC1 and write just ADC instead. ESP32 and ESP32s3 have also ADC2, I haven't yet looked code in details but don't think it is usefull to qualify the number of ADC ports set in RG_GAMEPAD definition. What do you think?

Akrobate commented 5 months ago

@ducalex Hello, I tested your implementation, everything works ok!

there was just a very little mistake:

+    {RG_KEY_UP,    ADC1_CHANNEL_5, ADC_ATTEN_DB_11, 3072, 4096},\
+    {RG_KEY_RIGHT, ADC1_CHANNEL_6, ADC_ATTEN_DB_11, 1024, 3072},\   
+    {RG_KEY_DOWN,  ADC1_CHANNEL_5, ADC_ATTEN_DB_11, 3072, 4096},\  <== here should be 1024, 3072
+    {RG_KEY_LEFT,  ADC1_CHANNEL_6, ADC_ATTEN_DB_11, 1024, 3072},\ <== here should be 3072, 4096

I also removed all my modifications on docker, docker-compose and building doc.

Akrobate commented 5 months ago

Photos are a really cool Idea! I will add mine a little later in an other PR if it's ok for you, I would like to add some schematics, and a README in my target.

But i'll work on it later

ducalex commented 5 months ago

Little suggestion in naming in RG_GAMEPAD_ADC1_MAP maybe you can remove ADC1 and write just ADC instead. ESP32 and ESP32s3 have also ADC2, I haven't yet looked code in details but don't think it is usefull to qualify the number of ADC ports set in RG_GAMEPAD definition. What do you think?

It's important to be able to qualify the ADC port because we can't determine it from the constant alone (because ADC1_CHANNEL_7 == ADC2_CHANNEL_7) and functions to access the ports are different. Esp-idf 5.0 has a nicer API to handle the ADCs but we have to support 4.x still. In practice I don't think the ADC2 will ever be used because it is quirky, so we could ignore it...

Also I had a more advanced version of my changes where I used a single map to reference all inputs but it made the code more complicated and used macros a lot which I usually try to avoid. So I pushed the simplified version. But maybe it's worth it, what do you think?

Declaration would look like this:

#define RG_GAMEPAD_KEYMAP {\
    {RG_KEY_UP,     ADC1(7, 3073, 4096)},\
    {RG_KEY_RIGHT,  ADC1(6, 1025, 3072)},\
    {RG_KEY_DOWN,   ADC2(7, 1025, 3072)},\
    {RG_KEY_LEFT,   ADC2(7, 3073, 4096)},\
    {RG_KEY_SELECT, GPIO(27, PULLUP, 0)},\
    {RG_KEY_START,  GPIO(39, PULLUP, 0)},\
    {RG_KEY_MENU,   GPIO(13, PULLUP, 0)},\
    {RG_KEY_OPTION, GPIO(0,  FLOAT,  0)},\
    {RG_KEY_A,      I2C(0x20, 1, PULLUP, 0)},\
    {RG_KEY_B,      I2C(0x20, 2, PULLUP, 0)},\
}

Which expands to roughly:

#define RG_GAMEPAD_KEYMAP {\
    {RG_KEY_UP,     1, .adc = {1, ADC1_CHANNEL_7, ADC_ATTEN_DB_11, 3073, 4096}},\
    {RG_KEY_RIGHT,  1, .adc = {1, ADC1_CHANNEL_6, ADC_ATTEN_DB_11, 1025, 3072}},\
    {RG_KEY_DOWN,   1, .adc = {2, ADC2_CHANNEL_7, ADC_ATTEN_DB_11, 1025, 3072}},\
    {RG_KEY_LEFT,   1, .adc = {2, ADC2_CHANNEL_6, ADC_ATTEN_DB_11, 3073, 4096}},\
    {RG_KEY_SELECT, 2, .soc = {GPIO_NUM_27, RG_GPIO_INPUT_PULLUP, 0}},\
    {RG_KEY_START,  2, .soc = {GPIO_NUM_39, RG_GPIO_INPUT_FLOAT,  0}},\
    {RG_KEY_MENU,   2, .soc = {GPIO_NUM_13, RG_GPIO_INPUT_PULLUP, 0}},\
    {RG_KEY_OPTION, 2, .soc = {GPIO_NUM_0,  RG_GPIO_INPUT_FLOAT,  0}},\
    {RG_KEY_A,      3, .i2c = {0x20, 1, RG_GPIO_INPUT_PULLUP, 0}},\
    {RG_KEY_B,      3, .i2c = {0x20, 2, RG_GPIO_INPUT_PULLUP, 0}},\
}
ducalex commented 5 months ago

there was just a very little mistake:

+    {RG_KEY_UP,    ADC1_CHANNEL_5, ADC_ATTEN_DB_11, 3072, 4096},\
+    {RG_KEY_RIGHT, ADC1_CHANNEL_6, ADC_ATTEN_DB_11, 1024, 3072},\   
+    {RG_KEY_DOWN,  ADC1_CHANNEL_5, ADC_ATTEN_DB_11, 3072, 4096},\  <== here should be 1024, 3072
+    {RG_KEY_LEFT,  ADC1_CHANNEL_6, ADC_ATTEN_DB_11, 1024, 3072},\ <== here should be 3072, 4096

Thanks for that, I fixed it in the other targets. Clearly I didn't test enough yesterday :)

Akrobate commented 5 months ago

It's important to be able to qualify the ADC port because we can't determine it from the constant alone (because ADC1_CHANNEL_7 == ADC2_CHANNEL_7) and functions to access the ports are different. Esp-idf 5.0 has a nicer API to handle the ADCs but we have to support 4.x still. In practice I don't think the ADC2 will ever be used because it is quirky, so we could ignore it...

Yes! You are absolutely right, I haven't seen the code when I wrote this suggestion.

About this part

#define RG_GAMEPAD_KEYMAP {\
    {RG_KEY_UP,     ADC1(7, 3073, 4096)},\
    {RG_KEY_RIGHT,  ADC1(6, 1025, 3072)},\
    {RG_KEY_DOWN,   ADC2(7, 1025, 3072)},\
    {RG_KEY_LEFT,   ADC2(7, 3073, 4096)},\
    {RG_KEY_SELECT, GPIO(27, PULLUP, 0)},\
    {RG_KEY_START,  GPIO(39, PULLUP, 0)},\
    {RG_KEY_MENU,   GPIO(13, PULLUP, 0)},\
    {RG_KEY_OPTION, GPIO(0,  FLOAT,  0)},\
    {RG_KEY_A,      I2C(0x20, 1, PULLUP, 0)},\
    {RG_KEY_B,      I2C(0x20, 2, PULLUP, 0)},\
}

It's really very elegant approach! I do love it! But it will make the code a little more difficult to customize (I mean in rg_input.c). So I don't really have a definitive position on it. The way it is done now and just before is very straight forward and very clear. The advanced approach make configuration clearer but the code would be more complex under the hood.

So it's up to you. Both are good. Actually in a private repo I would do what you suggseted. But I an open source project I think it's interresting to keep things as simple as possible. (just a personnal opinion)

ducalex commented 5 months ago

Thanks for the valuable insight, for now I'll leave things as is!

The fancier code isn't actually much more complicated but I need to finalize rggpio* first, because and that will determine the format of the config.h. So if I merge now and change my mind in a few weeks, all targets will need to be updated again, including yours. Which will cause conflicts and unnecessary work for everybody.