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

WIP: support for Sequre S60P #1883

Merged
merged 37 commits into from
Feb 25, 2024
Merged

WIP: support for Sequre S60P #1883

merged 37 commits into from
Feb 25, 2024

Conversation

ArturoGuerra
Copy link
Contributor

  • Please check if the PR fulfills these requirements
  • [] The changes have been tested locally
  • [] There are no breaking changes
  • What kind of change does this PR introduce?
  • What is the current behavior?
  • What is the new behavior (if this is a feature change)?

  • Other information:

@ArturoGuerra
Copy link
Contributor Author

pdo selection still needs a lot of work should hopefully have that done this week

@discip discip requested a review from Ralim February 21, 2024 09:01
@discip discip marked this pull request as draft February 21, 2024 09:02
@discip
Copy link
Collaborator

discip commented Feb 21, 2024

@ArturoGuerra

Thank you very much for taking this on. 😃👍🏻

pdo selection still needs a lot of work should hopefully have that done this week

I've converted this into a draft for now.

@discip discip marked this pull request as ready for review February 22, 2024 06:59
@discip discip enabled auto-merge February 22, 2024 07:00
@discip
Copy link
Collaborator

discip commented Feb 22, 2024

@ArturoGuerra
Nice!
We just need @Ralim to have a look at it.

Have you tested this on your device?
Do you think this is ready to be implemented?

@ArturoGuerra
Copy link
Contributor Author

Not ready yet I have tested it and pdo selection still has some issues

@discip discip disabled auto-merge February 22, 2024 07:42
@discip
Copy link
Collaborator

discip commented Feb 22, 2024

@ArturoGuerra

Sorry for digging around in your PR. 😓

I initially tried to merge your fork s60p into mine, but somehow the changes endet up here in this PR.

While doing so, @Ralim merged a PR of his own in and that made the confusion perfect.

I hope you are not mad at me. Was just curious and wanted to test the progress you made on my device.

Hopefully you will be able to sort this out.

If you need me to test I'll be happy to help.

@ArturoGuerra
Copy link
Contributor Author

No worries I'll figure it out 😄

@ArturoGuerra
Copy link
Contributor Author

@Ralim @discip currently having an issue where I keep on triggering overcurrent protection so the iron keeps restarting, what do I need to tweak to fix that issue? I set the powerSupplyWattageLimit like you guys did here but it still happens

powerSupplyWattageLimit = ((20 * ilim) / 100) - 2; // We take off 2W for safety of overhead

@Ralim
Copy link
Owner

Ralim commented Feb 23, 2024

If its working fine apart from over current tripping, I'm happy to look into that. I want to re-work the PWM control of these devices anyway so I can sort that out then?

So long as the rough work of voltage & PDO selection is done, thats really the biggest pest.
Otherwise just make sure its rebased / mergable and let me know and I'll give a review.

@ArturoGuerra
Copy link
Contributor Author

Yeah voltage and PDO are working as expected now, using the same selection logic that was used for USBPD.cpp

@ArturoGuerra
Copy link
Contributor Author

Yes Sir.
I assume it has something to do with PPS. 🤷🏻

what I find interesting is that it falls back to QC since that protocol is not even enabled so it shouldn't be possible

can you check which pdo profiles it shows in the debug menu

@discip
Copy link
Collaborator

discip commented Feb 24, 2024

Sorry missed your post. 😅

State 1 0

5 0V 5.0A

@ArturoGuerra
Copy link
Contributor Author

Sorry missed your post. 😅

State 1 0

5 0V 5.0A

that shouldn't even be possible since the debug menu has a check for 0V so that shouldn't even be displayed, I'll add another item to the debug menu to check the current protocol hold on

@ArturoGuerra
Copy link
Contributor Author

@discip check with the latest commit the last value in the state debug screen should be the current protocol, 21 PD 20 PPS 4-6 for QC

@discip
Copy link
Collaborator

discip commented Feb 24, 2024

Back to the rebooting-loop.

Was able to get a peek in PD Debug:

first:
State 0 0 0

then:
State 0 0 21

@ArturoGuerra
Copy link
Contributor Author

can you try with this one S60P_EN.zip

@discip
Copy link
Collaborator

discip commented Feb 24, 2024

Unfortunately no change.

@ArturoGuerra
Copy link
Contributor Author

Unfortunately no change.

interesting cuz the power IC does make it to PD mode so the charger should send back the PDOs and we don't really need to worry about intercepting those in time as the power IC saves them so what I think is happening is that we are reading those registers too early let me tweak the code a bit and we'll try again

@ArturoGuerra
Copy link
Contributor Author

Unfortunately no change.

does this charger work with the original firmware btw?

@discip
Copy link
Collaborator

discip commented Feb 25, 2024

does this charger work with the original firmware btw?

Yes, it does.

@ArturoGuerra
Copy link
Contributor Author

@discip try with this new commit it should now read the PDO registers every time the power_check loop runs, should fix any timeout issues we might have had

@ArturoGuerra
Copy link
Contributor Author

@discip try with this new commit it should now read the PDO registers every time the power_check loop runs, should fix any timeout issues we might have had
S60P_EN.zip

@ArturoGuerra
Copy link
Contributor Author

@discip interesting found a charger that causes the exact behavior you are experiencing, looks like enabling the PD protocol causes the iron to reboot for some odd reason i'll try to figure it out

@ArturoGuerra
Copy link
Contributor Author

@discip changed the protocol selection timing it should work now

@ArturoGuerra ArturoGuerra requested a review from Ralim February 25, 2024 06:32
Copy link
Owner

@Ralim Ralim left a comment

Choose a reason for hiding this comment

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

Not sure why you changed up the Front.h file.
The rest looks great.

source/Core/BSP/Sequre_S60/Software_I2C.h Outdated Show resolved Hide resolved
source/Core/Drivers/Font.h Outdated Show resolved Hide resolved
@ArturoGuerra ArturoGuerra requested a review from Ralim February 25, 2024 09:03
@Ralim Ralim merged commit 9ea71bc into Ralim:dev Feb 25, 2024
17 checks passed
@Ralim
Copy link
Owner

Ralim commented Feb 25, 2024

Thank you massively for this work, it's extremely appreciated.

I'll try and look into tuning the pid and pwm this week

@discip
Copy link
Collaborator

discip commented Mar 27, 2024

@Ralim

... look into tuning the pid and pwm ...

Have you had time for this yet?

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.

3 participants