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

Touch Screen Mod on Horus X10S Express at low temperatures goes into "Wrong PCB detected" #4509

Closed
1 task done
robustini opened this issue Jan 6, 2024 · 28 comments · Fixed by #4529
Closed
1 task done
Labels
bug 🪲 Something isn't working triage Bug report awaiting review / sorting

Comments

@robustini
Copy link
Contributor

Is there an existing issue for this problem?

  • I have searched the existing issues

What part of EdgeTX is the focus of this bug?

Transmitter firmware

Current Behavior

PR #4151 (already merged) that allows using a touch screen lcd on FrSky X10 unfortunately conflicts multi-trainer feature, which contemplates removing a pin from the ISRM module, which is easy to do and which many have done and which I still use in the OpenTX era.
The result is that my X10S Express now reports "Wrong PCB" at power up, now the firmware thinks I applied the modification to the lcd, which is absolutely wrong.
I don't understand why, since the lcd touch screen modification is quite complicated, instead of doing the pcb check you didn't leave a compile flag for it, it would have been better imho, or the PCB reconnaissance should not be done if the firmware is compiled with "-DALLOW_TRAINER_MULTI=YES" as in my case.

Expected Behavior

With revert 77fd73 my tx turns on regularly, so please @MRC3742 fix this issue, as new implementations normally should not break old established functionality, thanks.

Steps To Reproduce

  1. Modify the ISRM module by removing the pin in the picture
  2. Compile the firmware with -DALLOW_TRAINER_MULTI=YES
  3. Turn on the TX

Version

Nightly (Please give date/commit below)

Transmitter

FrSky X10 Express / X10S Express (ACCESS)

Operating System (OS)

No response

OS Version

Irrilevant

Anything else?

294686395-b7b89f98-681d-4a0d-b208-837275129908

The pin to be removed for the Multi-Trainer.

294687051-55d23b1e-4764-4c0d-8535-295a1de8f465

@robustini robustini added bug 🪲 Something isn't working triage Bug report awaiting review / sorting labels Jan 6, 2024
@pfeerick
Copy link
Member

pfeerick commented Jan 6, 2024

Please make sure you submit all the details... you are running an X10S Express, not an X10... and that detail matters, as this is exactly what the PCB revision check is trying to detect in this instance, in order to show the TR_WRONG_PCBREV error.

Do you know what that pin on the ISRM board actually connects to on the mainboard / MCU? As the real question atm is why the PCB check now thinks you are running X10 Express firmware on a X10 ... i.e. does it think PA.06 is grounded? I can't see other reference to it being used for anything else for either the X10/X10Express, so that could either mean it actually is grounded on the X10 Express (unlikely IMO), or because you mentioned time was a factor, maybe the pin isn't initialised properly at startup, and is in an indeterminate state?

The PCB check is much older than the ALLOW_TRAINER_MULTI feature, and is not going away (but may need to be reverted to it's earlier form). What instead I would like to see happen here is you and @MRC3742 work together to resolve this, given he has both radio types, and you have one exhibiting this behaviour. So between you, you should be able see why this condition is tripping, and if it is merely a mistake in the check, or if it is not not possible to do this as a runtime test with this hardware. Looking at the code, I have a suspicion it is merely a bug in the test, but testing will confirm that.

IIRC, the revision pins are configured as

X10

  • PH.07 -pulled down
  • PH.08- pulled down

X10 Express

  • PH.07 - pulled up
  • PH.08 - floating

This became problematic once the touch screen was added since PH.07/PH.08 are used for the touch panel, so the resistors had to go, which would then trip the revision check. PA.06 should have been an alternate test (with had higher priority, since it was manually applied) but clearly this isn't working now.

This is the test responsible for this screen being shown:

#define PCBREV_VALUE() ((GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_7) + (GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_8) << 1)) * GPIO_ReadInputDataBit(PCBREV_TOUCH_GPIO, GPIO_Pin_6))

@MRC3742
Copy link
Contributor

MRC3742 commented Jan 7, 2024

Yes - I will be doing some testing tomorrow to hopefully see where the problem is happening.

You are correct as my X10 EXPRESS has (R2) PH.07 pulled up to 3.3v and PH.08 is floating. (There is no resistor at R1 pads)
@robustini can you confirm that your EXPRESS board has the same?
As referred to in this comment #4114 (comment), they are both at the lower left corner of the MCU.

Also if you build with "-DALLOW_TRAINER_MULTI=NO" will it then start normally?
When I originally submitted the touch PR, I built a test version for my X10S EXPRESS and it started correctly? 😕

I do have both an X10 ACCST version and a X10S EXPRESS model that I can test with.
What I do not have is an X10 EXPRESS ISRM Module with the pin removed for the multi trainer.

Thanks - Rich

@robustini robustini changed the title Touch Screen Mod on Horus X10 Transmitters break the Multi-Trainer feature Touch Screen Mod on Horus X10S Express Transmitters break the Multi-Trainer feature Jan 7, 2024
@robustini
Copy link
Contributor Author

robustini commented Jan 7, 2024

@pfeerick sorgive me that I did not state in the title that the X10S is Express, but the image I posted leaves little interpretation, it says "X10S EXPRESS", but I have now changed the title of the issue so that there is no misunderstanding.
As to which pin was removed to enable multi-trainer can surely answer the dear @raphaelcoeffic better than I can, since he was the one who pointed it out to me when back in the day worked on that implementation in OpenTX.
@MRC3742 thanks for reply, I am performing further tests and will post them here shortly.
I confirm that my board is that of the original "X10S Express" (see the logo in my photo), it is not an X10S with additional ISRM module, it is original Fr-Sky.

@robustini
Copy link
Contributor Author

robustini commented Jan 7, 2024

I have created three firmwares, one with the current main branch and the multi trainer active, one disabling it, and one that does not cover this offending commit.
I have flashed and powered on repeatedly without ever a problem with all the firmwares, but I am certain of the "pcb wrong" yesterday, the tx did not repeatedly power on as many times.
So in thinking back I'm reflecting on when I had that "pcb wrong," on two different days with two versions of the firmware compiled: the tx was in my garage for a whole night.
This is going to sound crazy but I'm starting to think that the RTC battery since the current temperature in my garage is no more than 5 degrees may have dropped as a voltage and at power up may have caused a problem... possible?
At this point I can find no other explanation, the only test I can do now is to take the tx back to the garage, wait until tomorrow, and try to turn it back on with the main branch firmware flashed now.
But I am sure of one thing: the date and time have always been correct.
I can't figure out what the cold can affect to trigger this problem if not the RTC battery, and I obviously ask for your opinion.
This morning at home with 20 degrees temperature, the RTC battery shows a voltage of 2.75V, definitely not the 3.3 of a fully charged battery.
Now the question is: if the RTC battery voltage drops a little, just below that value for cold weather can it cause a PCB wrong on power-up but leaving correct date and time?
If it was discharged it would also lose date and time, I have never had any reports of RTC battery low and I have always enabled the voltage check in SYS.
What else could the low temperature go into to trigger a PCB wrong at boot?
This morning I simply install a new RTC battery, take the tx to the garage and update tomorrow.
But certainly if this was the problem rather than "wrong PCB" I would have expected "RTC battery low".

@pfeerick
Copy link
Member

pfeerick commented Jan 7, 2024 via email

@pfeerick
Copy link
Member

pfeerick commented Jan 7, 2024 via email

@robustini
Copy link
Contributor Author

@pfeerick in fact I'm going crazy trying to understand what it is, but I'm sure that yesterday after doing a revert of that commit the tx started on the first try, as well as the other day, but in fact I had brought it home on both occasions cases, so the ambient temperature was higher.

@robustini
Copy link
Contributor Author

robustini commented Jan 7, 2024

After leaving the tx in the cold in the garage for an hour, flashed with the latest main branch, and running at home after multiple power-ups this is the result.

20240107_101209.mp4

And it kept giving "wrong PCB" in multiple power-ups, tried at least 10 times.
Leaving it off so that it would stay cool I then flashed the firmware without the touch screen commit, and this is the result.

20240107_102636.mp4

And I'm continuing to turn it off and on and it keeps working.
Moral: whether the cold weather does anything I don't know, no I think there is a timer that sends the PCB reconnaissance on the ball but one thing is certain: without that commit the tx works, all the time!
And now? ;-)
I have one last test left to do, I will now try to flash the firmware to the latest commit again and see what happens.

@robustini
Copy link
Contributor Author

Well, this is the 9 proof that I hope can leave no doubt.
Flashing now with "cold" tx the firmware without the lcd touch commit always works, flashing the one with that commit, so from the latest main branch does immediately wrong PCB, and it does this constantly by turning it back on.
Long video then posted on YouTube.
Do not ask me why or how come because I am unable to know, but without a shadow of a doubt it is so with my tx.
I look forward to your considerations.

https://youtu.be/3MtzdIZhn7k

@robustini
Copy link
Contributor Author

robustini commented Jan 7, 2024

RTC Battery has nothing to do with it, installed new, now voltage 3.27V but same problem.
So I really don't know how the cold here can affect this way with the touch screen mod, it can't change a pin value.
Now I'm taking the tx back to the house at a temperature of 20 degrees, I want to see what happens.
And assuming that it is a cold-related issue that maybe compromises a suboptimal soldering in the PCB somewhere, but I don't think so.
Maybe there really is "a time" after which the problem occurs, but always and only with that commit.

@robustini robustini changed the title Touch Screen Mod on Horus X10S Express Transmitters break the Multi-Trainer feature Touch Screen Mod on Horus X10S Express (with ISRM Multi-Trainer Mod) goes into PCB Wrong Jan 7, 2024
@robustini
Copy link
Contributor Author

robustini commented Jan 7, 2024

Do you want to laugh?
Tx acclimated always with firmware giving "wrong PCB", now it turns on regularly, LOL!
Please make the screen touch mod to be declared in compilation or even other, or I really don't know what to do except sell the tx to someone who lives in the Canary Islands! :-)

@robustini
Copy link
Contributor Author

robustini commented Jan 7, 2024

To momentarily curb the problem here I removed the possibility that it could intercept my tx with the touch screen, by modifying in my compilation batch the hal.h file like this:

sed -i 's/((GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_7) + (GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_8) << 1)) * GPIO_ReadInputDataBit(PCBREV_TOUCH_GPIO, GPIO_Pin_6))/(GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_7) + (GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_8) << 1))/' /home/marco/src/edgetx/edgetx_main/radio/src/targets/horus/hal.h

But curbing doesn't mean solving, but at least then they i can stay up to date with new commits without EdgeTX thinking I have a touch screen lcd.

@MRC3742
Copy link
Contributor

MRC3742 commented Jan 8, 2024

@robustini
Great troubleshooting figuring out what is causing your problem with tripping the PCBREV check at startup!

@pfeerick
After reviewing the above posts the problem has to be at the floating MPU pin 55 (PA.06)
Evidently when in a cold temperature state it has a data bit value of zero but as it warms up it "floats" to a higher value.
This is definitely an issue against the board check status I proposed in the X10 Touch Update PR.

Making the Touch Update as an optional compile flag won't resolve this problem as the board check will still fail.
I think the PCBREV check is looking for a data bit of zero for the NON-EXPRESS and one or greater for the EXPRESS ?

I propose that an added requirement for Touch Update on the EXPRESS could be to add a 1K pullup to 3.3v on Pin55 (PA.06)
Then with an optional compile flag for touch on X10, I could add the code to in hal.h to the //PCBREV section that starts with:
#if defined(PCBX10) && defined(HARDWARE_TOUCH) adding below the code for the pin check of PA.06
Then use the earlier original code starting with #elif defined(PCBX10).

Does this make sense or do you see a better option?

Thanks - Rich

@pfeerick
Copy link
Member

pfeerick commented Jan 8, 2024

The four step test that is causing looks to effectively be looking for a value of 0 for NON-EXPRESS and 3 for EXPRESS ... no resistor or two resistors (since two bits in binary == 3). Considering the initial state of the GPIOs (and whether actually initalised, or pulled up or down) is also important. Have a look at step 2... it initialises the GPIO, and because it only init PCBREV_GPIO / PCBREV_GPIO_PIN, PCBREV_TOUCH_GPIO / PCBREV_TOUCH_GPIO_PIN is never initialised. That may or not be a problem here, but probably isn't a good state to be in. If taking all of that into consideration, you can knock some sense into this, that would be great, Otherwise, for now at least, yes an additional compile flag may be needed. I would have preferred a runtime option, but it's too late in the cycle to contemplate going there.

I'd be curious to know what happens if an additional

#if defined(PCBREV_TOUCH_GPIO_PIN)
  GPIO_InitStructure.GPIO_Pin = PCBREV_TOUCH_GPIO_PIN;
  GPIO_Init(PCBREV_TOUCH_GPIO, &GPIO_InitStructure);
#endif

was added just before

hardwareOptions.pcbrev = PCBREV_VALUE();

Would you both mind trying this build of 2.10 on your X10 Express? It compiles but that is the extent of my ability to test it. 🤭 I've also added the X10 ACCST build of the same code for comparison.

x10-access-fb28d2e75.zip
x10-fb28d2e75.zip

Step 1

#define PCBREV_VALUE() ((GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_7) + (GPIO_ReadInputDataBit(PCBREV_GPIO, GPIO_Pin_8) << 1)) * GPIO_ReadInputDataBit(PCBREV_TOUCH_GPIO, GPIO_Pin_6))

Step 2

#elif defined(PCBREV_GPIO_PIN)
#if defined(PCBREV_GPIO_PULL_DOWN)
GPIO_ResetBits(PCBREV_GPIO, PCBREV_GPIO_PIN);
GPIO_InitStructure.GPIO_PuPd = GPIO_PuPd_DOWN;
#endif
GPIO_InitStructure.GPIO_Pin = PCBREV_GPIO_PIN;
GPIO_Init(PCBREV_GPIO, &GPIO_InitStructure);
hardwareOptions.pcbrev = PCBREV_VALUE();

Step 3

// PCBREV driver
enum {
// X12S
PCBREV_X12S_LT13 = 0,
PCBREV_X12S_GTE13 = 1,
// X10
PCBREV_X10_STD = 0,
PCBREV_X10_EXPRESS = 3,
};
#if defined(SIMU)
#define IS_FIRMWARE_COMPATIBLE_WITH_BOARD() true
#elif defined(PCBX10)
#if defined(PCBREV_EXPRESS)
#define IS_FIRMWARE_COMPATIBLE_WITH_BOARD() (hardwareOptions.pcbrev == PCBREV_X10_EXPRESS)
#elif defined(RADIO_FAMILY_T16)
#define IS_FIRMWARE_COMPATIBLE_WITH_BOARD() (true)
#else
#define IS_FIRMWARE_COMPATIBLE_WITH_BOARD() (hardwareOptions.pcbrev == PCBREV_X10_STD)
#endif

Step 4

edgetx/radio/src/opentx.cpp

Lines 1588 to 1590 in fb28d2e

if (!IS_FIRMWARE_COMPATIBLE_WITH_BOARD()) {
runFatalErrorScreen(STR_WRONG_PCBREV);
}

@MRC3742
Copy link
Contributor

MRC3742 commented Jan 8, 2024

Thanks for the comprehensive and complete explanation!
I now understand most of it with a few areas still being a little beyond my basic coding skills!!! 🧠

I have tested the firmware from your post as you requested.
They start and run with no errors on both the NON-EXPRESS and EXPRESS models.
Not sure I'm a big help here as they also both will start and run with the current main branch builds.

My NON-EXPRESS with the touch panel mod starts and is working as expected.
SYS/HARDWARE/ANALOGS screen reports,
"Touch Panel: 200 : 130" ( X and coordinates change properly with finger movement)
"Touch GT911 FW ver: 4192 TSI2CEvents: 0"

My EXPRESS with no modifications or touch panel added (yet) starts and is working as expected.
SYS/HARDWARE/ANALOGS screen reports,
(Coordinates area is blank as touch panel is not installed)
"Touch GT911 FW ver: 0 TSI2CEvents: 0"

Thanks Again for your time and effort on this project - Rich

@pfeerick
Copy link
Member

pfeerick commented Jan 8, 2024 via email

@robustini
Copy link
Contributor Author

robustini commented Jan 8, 2024

@MRC3742 was the only way I could continue to use the tx and try to figure out where the problem was, definitely not very elegant but functional as it is automated in my personal employment.
@pfeerick flashed your firmware, tx being frozen, i will keep you updated on the result soon.

@robustini
Copy link
Contributor Author

robustini commented Jan 8, 2024

@pfeerick :-(
Now I keep the tx in the cold for future attempts.

damn2.mp4

@robustini robustini changed the title Touch Screen Mod on Horus X10S Express (with ISRM Multi-Trainer Mod) goes into PCB Wrong Touch Screen Mod on Horus X10S Express (with ISRM Multi-Trainer Mod) goes into "Wrong PCB detected" Jan 8, 2024
@pfeerick
Copy link
Member

pfeerick commented Jan 8, 2024

Well, that also rules out anything on the ALLOW_TRAINER_MULTI path also, as that wasn't enabled for either of those firmwares.

So yes, it looks like Plan B ... optional compile flag ... will be needed (for now at least) so we don't break X10 Express for everyone else. 😢

@robustini
Copy link
Contributor Author

robustini commented Jan 9, 2024

@pfeerick provided of course this is not due to a problem with my tx, I don't know, it doesn't mention any malfunction.
I wouldn't want it to be a false positive anyway, although here systematically at very cold tx it happens.
@MRC3742 should try yours by putting it in the refrigerator for a couple of hours.
To understand if there is actually a problem with a floating pin based on the temperature of the PCB.

@MRC3742
Copy link
Contributor

MRC3742 commented Jan 10, 2024

I can confirm that my X10 EXPRESS when subjected to cold, 32degees F. (0 degrees C.) ❄️ , will not start and produces the same "Wrong PCB detected" error. This is surely being caused by floating pin 55 on the EXPRESS changing its state and tripping the detection.

Working on a PR to make the Touch Mod optional (Plan B) for the X10 series and revert the PCBREV check back to its original format when firmware is built without the "HARDWARE_TOUCH=ON" option.

EDIT: Test firmware build for X10 EXPRESS with Allow_Multi_Trainer build option.

Rich

@robustini robustini changed the title Touch Screen Mod on Horus X10S Express (with ISRM Multi-Trainer Mod) goes into "Wrong PCB detected" Touch Screen Mod on Horus X10S Express goes into "Wrong PCB detected" Jan 10, 2024
@robustini robustini changed the title Touch Screen Mod on Horus X10S Express goes into "Wrong PCB detected" Touch Screen Mod on Horus X10S Express at low temperatures goes into "Wrong PCB detected" Jan 10, 2024
@robustini
Copy link
Contributor Author

@MRC3742 thank you very much for the test, because initially I too had begun to doubt that I had defective hardware.
It is not easy to convince someone that they have found a problem, especially when these almost absurd things occur.
Just think if I was here in the summer, the problem would not have come out and in version 2.10 so many would have had this problem.

@MRC3742
Copy link
Contributor

MRC3742 commented Jan 10, 2024

Glad you were able to catch it before the 2.10 release for sure!

I've added a test firmware to the above post that was built on Gitpod from my branch using:
cmake -Wno-dev -DPCB=X10 -DPCBREV=EXPRESS -DALLOW_TRAINER_MULTI=ON -DCMAKE_BUILD_TYPE=Release ../

@MRC3742
Copy link
Contributor

MRC3742 commented Jan 10, 2024

@robustini Can you confirm that resistor R1 is not present and the pads for it are not populated on your EXPRESS board?
Attached picture R1 is located lower left side of processor.

I'm still trying to understand why if Pin 98 (PH.08) is also floating in the original PCBREV check, why that pin isn't effected by the cold temperature that is causing floating Pin 55 (PA.06) to change its state from one to zero?

X10-Express_MCU

Thanks - Rich

@robustini
Copy link
Contributor Author

@MRC3742 I will open the tx again tomorrow and give you confirmation, but if yours is an Express like mine I don't see how mine can have the R1 welded on.

@MRC3742
Copy link
Contributor

MRC3742 commented Jan 11, 2024

Thanks - Just curious if mine was missing when built?
I don't think R1 can be populated on the EXPRESS board because the empty pad opposite the processor pin is ground.
I believe a resistor there would pull that pin down and cause the PCBREV check to fail.

I've updated the PR for the fix to enable the processors internal pull-up resistor on the problem floating Pin 55 (PA.06)
This has fixed my X10 EXPRESS and have tested down to 0 degrees F. (-18 C.) with all normal startups.
Here is the firmware built by the updated PR's Actions page for your transmitter if you have a chance to test it.
x10-access-bc85dfa.zip

Thanks for your testing and input - Rich

@robustini
Copy link
Contributor Author

robustini commented Jan 11, 2024

@MRC3742 the replacement mainboard for the X10S Express does not have R1, so neither does mine.

b06cf773b4c2ee242ef942b769c3e0b0

And finally your implementation works, thanks!
So closed the issue, hope for a merge soon.
Fixed with #4529.

@Sokatgithub
Copy link

How do you connect touch panel small ribbon cable. Do you have to make small pcb or can buy onr
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working triage Bug report awaiting review / sorting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants