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

Update touchpad driver and automatically detect errors #489

Merged
merged 10 commits into from
Oct 10, 2021
Merged

Update touchpad driver and automatically detect errors #489

merged 10 commits into from
Oct 10, 2021

Conversation

Riksu9000
Copy link
Contributor

@Riksu9000 Riksu9000 commented Jul 14, 2021

Cleaned up unused parts.
isTouch was buggy. Replaced with isValid and touching.
Already added in #492
Read information about the chip and display it in SystemInfo in format ChipId.VendorId.FwVersion.

info

@Riksu9000 Riksu9000 marked this pull request as draft July 15, 2021 20:23
@JF002
Copy link
Collaborator

JF002 commented Jul 25, 2021

UPDATE: Another PR might replace this soon.
Is this #492 ? If so, can we close this one?

@Riksu9000
Copy link
Contributor Author

#492 doesn't show the information about the chip. I'll update this after #492 is handled in case we want to show this information. If we don't need to show this information, then this can be closed.

@JF002
Copy link
Collaborator

JF002 commented Jul 28, 2021

There's no actual need to display the info about the chip as the Pinetime always use the same chip, but it could help to debug issues with the touchpanel (factory installs another chip, chip fails to initialize,... -> display "???" or "Error" in that case, for example).

@Riksu9000
Copy link
Contributor Author

It seems that this information can be useful to detect faulty hardware #590.

@Riksu9000
Copy link
Contributor Author

I've added error checking on boot. In case the touch controller doesn't respond as expected, this screen will be shown.

error

@Riksu9000 Riksu9000 marked this pull request as ready for review August 18, 2021 12:33
@Riksu9000 Riksu9000 changed the title Update touchpad driver Update touchpad driver and automatically detect errors Aug 18, 2021
This was referenced Sep 11, 2021
@ildar
Copy link

ildar commented Sep 14, 2021 via email

@lman0
Copy link

lman0 commented Sep 21, 2021

I've added error checking on boot. In case the touch controller doesn't respond as expected, this screen will be shown.

error

@Riksu9000 i don't think it's useful to add a rectangular button in order to "proceed" because the user will be unable to click on it since the touch controller don't work to begin with.

it's better to remove the button (that's make the user angry since he can't click on it how much he want/try )
and replace it by a 15-30s timer , then proceed to main screen automatically .

so the user can use infinitime , as an smartwatch (the watch can still be able to connect to gadgetbridge for time sync)

but the user might be unable to validate the firmware regardless , since it need a functional touch controller.

this pull may be usefull for pine64 , for detecting faulty watch and prune them instead of sending to user

@Riksu9000
Copy link
Contributor Author

i don't think it's useful to add a rectangular button in order to "proceed" because the user will be unable to click on it since the touch controller don't work to begin with.

The screen can be closed by pressing the physical button.

@lman0
Copy link

lman0 commented Sep 21, 2021

@Riksu9000 it's still redundant then because , as user , i will try first to press the button "proceed "
and then after some angry try ,
i will , at last, try to click on the physical button as last try....

the "proceed" button feel like a nagging button and that's not good for someone that already have a bad product...

@Riksu9000
Copy link
Contributor Author

@Riksu9000 it's still redundant then because , as user , i will try first to press the button "proceed "
and then after some angry try ,
i will , at last, try to click on the physical button as last try....

the "proceed" button feel like a nagging button and that's not good for someone that already have a bad product...

This screen can be used to warn about other errors as well, in which case the proceed button makes more sense. Also touch could still just work if the warning is a false positive.

The user might miss the warning entirely if there was a timeout, so I think it should require confirmation from the user.

@hubmartin
Copy link
Contributor

As an owner of PineTime and Colmi P8. Acording to this message, this warning screen appears on Colmi P8 everytime after boot.
#492 (comment)

I see InfiniTime project should be more open to be ported on other devices. I feel like this might hurt/annoy more people developing on other platforms or nrf52 kits.

If this was created because of one person was having issues, then if anyone else will ever ask, we can guide him to this thread where you created firmware showing those 3 registers on screen. #590 (comment)

Does this check really needs to be so strict on reading 3 internal registers (if I checked it right). Wouln't be enought to check whether the touch screen I2C address sends ACK so the sensor is there and correctly connected to the I2C?

@Riksu9000
Copy link
Contributor Author

InfiniTime officially only supports the PineTime and the Cst816 controller currently. The controller used in the P8 isn't compatible with the new touch handler currently, so showing a warning because of an unsupported controller is appropriate isn't it?

If official support for the Cst716 controller was added, then that controller could be whitelisted by allowing fwVersion to be either 1 or 2. 1 is for Cst816 and 2 is for Cst716.

@JF002
Copy link
Collaborator

JF002 commented Oct 5, 2021

@Riksu9000 & @hubmartin
Indeed, for now, InfiniTime officially only supports the PineTime. This is a deliberate choice because I cannot support multiple device on my own (more about this just below :) ).
Also, I know that Pine64 have already had to refuse a batch of PineTime coming from the factory because the wrong touch sensor was mounted on the PCB (a cheaper version of the CST816S). I have also received 2 engineering samples from the factory that had the wrong touch sensor. Now, I don't know if devices with the wrong sensor were actually shipped to the end-users, but I can only say that it's a possibility that the factory might install the wrong sensor by accident.
In this context, I think this PR makes sense, and will considerably help the user and the support if this would happen.

Now, I know that @hubmartin is very involved in a port of InfiniTime on the ColmiP8, and I really appreciate that! I would like to explore solutions that would allow us to continue working on InfiniTime for the PineTime while allowing some contributors to maintain ports for other targets. Should these port be forks of InfiniTime, completely independent from this project, or should InfiniTime be slightly refactored to be more easily ported across different devices? My long term dream is to port InfiniTime to the BL60x MCU, so porting to the P8 (which is a lot more similar to the PineTime than any BL60x board) might be a first step to this goal.
I'm not promising anything, but I think we should be able to find a nice compromise here ;)

@hubmartin
Copy link
Contributor

After thinking, I don't know I'll find time and motivation to fix cst718 driver for P8. It was nice bonus, that it worked before touch driver update. I understand your primary focus on your PineTime hardware.
I guess that refactoring InfiniTime for more generic drivers, which are not tied to the chips, will be needed as a first step to make it more universal. Forking is not good way in my opinion. But this driver refactoring is beyond my ability. So I'll keep P8 aside for now and will hope to find another PineTime somewhere in EU to buy second piece for debugging, instead of P8.

@ildar ildar mentioned this pull request Oct 8, 2021
@JF002 JF002 merged commit 6d0e68d into InfiniTimeOrg:develop Oct 10, 2021
@JF002 JF002 added this to the 1.7.0 milestone Oct 10, 2021
@JF002
Copy link
Collaborator

JF002 commented Oct 10, 2021

Thanks, @Riksu9000 !

@Riksu9000 Riksu9000 deleted the update_touch_driver branch October 20, 2021 16:06
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.

5 participants