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

Add HX8357D + STMPE610 drivers #64

Merged
merged 10 commits into from
Mar 4, 2020
Merged

Add HX8357D + STMPE610 drivers #64

merged 10 commits into from
Mar 4, 2020

Conversation

danjulio
Copy link
Contributor

@danjulio danjulio commented Mar 3, 2020

Please let me know if you have any questions or concerns.

Regards, Dan

@C47D C47D mentioned this pull request Mar 3, 2020
@C47D
Copy link
Collaborator

C47D commented Mar 3, 2020

Hi,

Thanks for the PR!, I will be able to check it later today. There are some conflics but I'm not 100% sure why, anyways, I can solve them on the web editor.

EDIT
Did a quick scan of the conflics and it seems like your work is a couple of commits behind master, I'm not sure what's the easiest approach here, rebase your repo and fix the remaining issues (like assigning 3 to the HX8357D controller instead of 2, given to ST7789).

Or apply some cherry picking of commits and solve the issues locally.

Are you familiar with this kind of issues?

Regards,
Carlos

@danjulio
Copy link
Contributor Author

danjulio commented Mar 3, 2020

Hey Carlos,

I apologize that I am pretty much a newbie at git. I may have screwed things up in my own repository because I can't seem to get it to try to merge with the master so I could catch up and then issue another pull request. I'm going to suggest how you resolve the various conflicts below but if that's too much then maybe I can just start fresh with your latest work and add the drivers and make the other modifications. Let me know what you'd prefer.

And yes, we can give the index 3 to the HX8357 driver since you've added the ST7789. Shouldn't be any problem. You'll see that I made constants and used those everywhere. Maybe those are useful too.

  1. disp_driver.c/disp_driver.h - Take my changes calling the disp_spi_init but make sure the ST7789 is included. I put the disp_spi_init in here to simplify main.c so added the flag to the disp_driver_init call.
  2. disp_spi.c/disp_spi.h - Perhaps we need a discussion about this. I didn't understand what the chained_post_cb did and it caused my ESP32 to crash (using the v3.3 IDF) so I removed it. The code will worked fine with the ILI9341 and HX8357D display chips (I don't have a ILI9488 to test with). Other changes in here are to include selecting SPI bus and the clock rate change.
  3. touch_driver.c/touch_driver.h - I made the same change allowing conditional SPI bus initialization to match the disp_driver as well as constants for the devices.
  4. tp_spi.c/tp_spi.h - Support for the STMPE610 and selecting SPI busses as well as a rewrite to use half-duplex transactions (although I left the original xchg function in there). I think you should be able to take my changes.
  5. xpt2046.c - changed to use the new functions in tp_spi.
  6. main.c - Added support for selecting SPI bus (and a check that the user selected the same bus if they're using the same pins). Simplified the intialization code by using the same driver init (but passing in the flag telling it that either we already initialized the SPI bus or that it needs to).

Obviously you can ignore my sdkconfig/sdkconfig.old. I also added the picture of my board and a description to the readme.md file that I think should be included.

The kconfig files have my changes and they work but as we discussed you should probably review them.

Also if you'd like, email me at the address in my github repository with your phone number and a good time to call and perhaps we can chat over the phone to come up with a plan.

Regards, Dan

@danjulio
Copy link
Contributor Author

danjulio commented Mar 3, 2020

Hang on Carlos! I did manage to resync my fork. :-) Let me poke around and see if i can resolve this stuff. Then I can issue another pull request that hopefully will be simpler for you to merge.

@C47D
Copy link
Collaborator

C47D commented Mar 3, 2020

Great, dont' worry I'm also new at git.

Obviously you can ignore my sdkconfig/sdkconfig.old.
I'm planning to ignore those files, because as far as I know they are not really neccesary.

I also added the picture of my board and a description to the readme.md file that I think should be included.

Great pic :)

The kconfig files have my changes and they work but as we discussed you should probably review them.

I will, I learned a new trick last weekend.

Also if you'd like, email me at the address in my github repository with your phone number and a good time to call and perhaps we can chat over the phone to come up with a plan.

I can't right now, I'm at work, but we can find another way to talk in "real time". I'm also a non english native speaker so you might have a hard time undertanding my broken english.

Hang on Carlos! I did manage to resync my fork. :-) Let me poke around and see if i can resolve this stuff. Then I can issue another pull request that hopefully will be simpler for you to merge.

I will hang on, I'm at work (don't tell anybody), so i can test the PR until later tonight.

Regards :)

@danjulio
Copy link
Contributor Author

danjulio commented Mar 3, 2020

Can you explain what the idea was behind the chained_post_cb in tp_spi is? I'm not sure what it does (and it caused me problems).

@C47D
Copy link
Collaborator

C47D commented Mar 3, 2020

It was introduced in #55 but explained in #54

Chaining to an optional user-supplied post_cb in disp_spi.c so that spi_ready can remain a private function

Afaik it allows the user to specify a callback to be called when the spi transaction is done, you can check the ESP32 docs for it here:

transaction_cb_t post_cb
Callback to be called after a transmission has completed.

This callback is called within interrupt context should be in IRAM for best performance, see “Transferring Speed” section in the SPI Master documentation for full details. If not, the callback may crash during flash operation when the driver is initialized with ESP_INTR_FLAG_IRAM.

@danjulio
Copy link
Contributor Author

danjulio commented Mar 3, 2020

Ok Carlos, I merged with the base branch and tested on my driver combination. One thing to note is that I changed the way you included files in the two drivers. I include all device files in the driver instead of conditionally including them because I could not get my project to compile that way. I don't think this should add any significant risk. Hopefully this is easier for you to integrate. Please let me know if you have any questions.

@danjulio
Copy link
Contributor Author

danjulio commented Mar 4, 2020

@C47D - just notifying you in case you didn't see the comment I left a bit ago. I think I updated everything in the pull request to make integration easier.

@C47D
Copy link
Collaborator

C47D commented Mar 4, 2020

Great, just arrived home, I will integrate it tomorrow, nice to see there are no more conflicts. Thanks for the work and patience!

Is everything clear with the post_cb? I think we can even use it to handle errors

@C47D
Copy link
Collaborator

C47D commented Mar 4, 2020

Let's merge and fix any issues if any. Thanks again for your contribution @danjulio !

@C47D C47D merged commit 2d30cba into lvgl:master Mar 4, 2020
@danjulio
Copy link
Contributor Author

danjulio commented Mar 4, 2020

@C47D, thank you Carlos! This port got me on the road of using the ESP32 and lvgl originally. My pleasure to give back a little. Next time you update, you might add the ST chip to the readme so people are aware. Cheers!

@C47D
Copy link
Collaborator

C47D commented Mar 4, 2020

Next time you update, you might add the ST chip to the readme so people are aware.

Just did, we're testing it.

Regards :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants