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

ae89a65 breaks serial (not usb) communication for lpc1768 #4194

Closed
pulsar256 opened this issue Apr 20, 2021 · 16 comments
Closed

ae89a65 breaks serial (not usb) communication for lpc1768 #4194

pulsar256 opened this issue Apr 20, 2021 · 16 comments
Labels
bug resolved Issue is thought to now be fixed

Comments

@pulsar256
Copy link

pulsar256 commented Apr 20, 2021

Serial Communication with default settings (250000 baud) is not working starting with commit-id ae89a65

verified by bisecting twice resulting in the same identified change:

➜  klipper git:(6cab7bcf) ✗ git bisect good
ae89a65956f95c2bee652396286d97b47ff804b6 is the first bad commit
commit ae89a65956f95c2bee652396286d97b47ff804b6
Author: Kevin O'Connor <kevin@koconnor.net>
Date:   Wed Mar 24 19:49:08 2021 -0400

    lpc176x: Do not modify PCLKSELx at runtime

    The lpc176x has an errata that could cause updates to PCLKSELx to not
    take effect.  Rework the code to use the default peripheral clock
    speed (25Mhz or 30Mhz) so that this register does not need to be
    updated at runtime.

    Signed-off-by: Kevin O'Connor <kevin@koconnor.net>

:040000 040000 4fb3f0682de8801a8e69d228e23a8de3d21e80c0 44aba2445b47ca4338c21d5c420dc1b5011f6fd4 M      src

Testing-Environment:

  • RPI4
  • BTT SKR 1.3
  • No Bootloader (flashing over RPI4 Serial)
  • Also using RPI4 Native Serial to connect to Klipper.

reverting the ae89a65 against current master (8f9e497) restores the functionality.

@pulsar256 pulsar256 changed the title 8a3a32058f2a4ec599d3542d154a6eecbd1b3b85 breaks serial (not usb) communication for lpc1768 ae89a65956f95c2bee652396286d97b47ff804b6 breaks serial (not usb) communication for lpc1768 Apr 20, 2021
@pulsar256 pulsar256 changed the title ae89a65956f95c2bee652396286d97b47ff804b6 breaks serial (not usb) communication for lpc1768 ae89a65 breaks serial (not usb) communication for lpc1768 Apr 20, 2021
@KevinOConnor
Copy link
Collaborator

I can confirm that commit is not correct for serial support. After the above change, a baud of 250000 would require a serial clock divisor of 6.25, and the code does not work correctly with fractions like that. (Previously, the peripheral clock was 4 times faster, and a divisor of 25 was fine.)

I'll need to think on how to fix that.

-Kevin

@dianlight
Copy link
Contributor

dianlight commented Apr 28, 2021

As a temporary workaround just use a baud rate of 256000 where the divisor is 6.1035....

@KevinOConnor To keep the 250000 I found no other solution than to modify the clock as before.
In any case, looking at the specifications of the lpc176x processor/libraries I noticed that the get_pclock_frequency should be modified to be more generic.

  • Use LCP_SC->PCLKSEL0 register to understand the divider
  • Read the CoreClock from the SystemClock variable instead of CONFIG_CLOCK_FREQ

The code would look something like this:

// Return the frequency of the given peripheral clock
uint32_t
get_pclock_frequency(uint32_t pclk)
{
    uint32_t var_UartPclk_u32,var_Pclk_u32;
    var_UartPclk_u32 = (LPC_SC->PCLKSEL0 >> 6) & 0x03;
    switch( var_UartPclk_u32 )
    {
          case 0x00:
            var_Pclk_u32 = SystemCoreClock/4;
            break;
          case 0x01:
            var_Pclk_u32 = SystemCoreClock;
            break; 
          case 0x02:
            var_Pclk_u32 = SystemCoreClock/2;
            break; 
          case 0x03:
            var_Pclk_u32 = SystemCoreClock/8;
            break;
    }      
    return var_Pclk_u32;
}

If you agree I send a PR.

L.

@pulsar256
Copy link
Author

I had a laymans look at the errata which suggest, that one should not change the PCLKSELx registers after the PLL0 is connected. I take it, the initialization order is not easy to rework / implement?

@Gfermoto
Copy link

Gfermoto commented May 1, 2021

256000 did not help (lpc1769)

@om2kw
Copy link

om2kw commented May 1, 2021

baudrate 230400 is working with lpc1769 (skr1.4Turbo)

@jakep82
Copy link

jakep82 commented May 4, 2021

Is their any reason the original commit can't be reverted? A recent youtube video showed how to connect a Pi Zero to the SKR boards via UART, and now we're getting a wave of support requests because it doesn't work at the default 250000 baud.

@Sineos
Copy link
Collaborator

Sineos commented May 4, 2021

#4248 (comment)

@KevinOConnor
Copy link
Collaborator

FYI, I have put together a possible fix on the work-lpcuart-20210504 branch (commit 191c358). Unfortunately, I can't test this right now.

cd ~/klipper ; git fetch ; git checkout origin/work-lpcuart-20210504 ; sudo service klipper stop ; make ; make flash ; sudo service klipper start

-Kevin

@pulsar256
Copy link
Author

Works for me:

➜  klipper git:(work-lpcuart-20210504) ✗ make clean
➜  klipper git:(work-lpcuart-20210504) ✗ make
➜  klipper git:(work-lpcuart-20210504) ✗ flash.sh
lpc21isp version 1.97
File /home/pi/klipper/out/klipper.bin:
        loaded...
        image size : 19152
Image size : 19152
Synchronizing (ESC to abort). OK
Read bootcode version: 2
4
Read part ID: LPC1768, 512 kiB FLASH / 64 kiB SRAM (0x26013F37)
Will start programming at Sector 1 if possible, and conclude with Sector 0 to ensure that checksum is written last.
Wiping Device. OK
Sector 1: ..............................................................................................
Sector 2: ..............................................................................................
Sector 3: ..............................................................................................
Sector 4: ..................................................................
Sector 0: ..............................................................................................
Download Finished... taking 1 seconds
Now launching the brand new code
➜  klipper git:(work-lpcuart-20210504) ✗ sudo systemctl start klipper.service moonraker.service
Tue, 04 May 2021 14:22:15 GMT > status
Tue, 04 May 2021 14:22:15 GMT < ok
Tue, 04 May 2021 14:22:15 GMT < // Klipper state: Ready

@pulsar256
Copy link
Author

P.S. might come handy for testing / flashing over the serial port (requires reset pin and boot pin to be connected to the RPI)

flash.sh

#!/bin/bash

pin_reset=22
pin_boot=26
firmware=$HOME/klipper/out/klipper.bin

sudo systemctl stop klipper

# enter boot bootloader sequence
gpio mode $pin_reset OUT
gpio mode $pin_boot  OUT

# hold reset and bootloader down
gpio write $pin_reset 0
gpio write $pin_boot 0

# go back to high-z and let skr pullups set the lines to high
gpio mode $pin_reset IN
gpio mode $pin_boot IN

lpc21isp -wipe -bin $firmware /dev/ttyS0 230400 12000

sudo systemctl start klipper

@harjoat
Copy link

harjoat commented May 4, 2021

FYI, I have put together a possible fix on the work-lpcuart-20210504 branch (commit 191c358). Unfortunately, I can't test this right now.

cd ~/klipper ; git fetch ; git checkout origin/work-lpcuart-20210504 ; sudo service klipper stop ; make ; make flash ; sudo service klipper start

-Kevin

Tried it, worked for me with an skr 1.4 turbo.

@dw-0
Copy link
Contributor

dw-0 commented May 4, 2021

FYI, I have put together a possible fix on the work-lpcuart-20210504 branch (commit 191c358). Unfortunately, I can't test this right now.

cd ~/klipper ; git fetch ; git checkout origin/work-lpcuart-20210504 ; sudo service klipper stop ; make ; make flash ; sudo service klipper start

-Kevin

Tested with a SKR 1.4 Turbo connected to a Pi Zero W via UART -> i can confirm it is working.

@totolook
Copy link

totolook commented May 4, 2021

FYI, I have put together a possible fix on the work-lpcuart-20210504 branch (commit 191c358). Unfortunately, I can't test this right now.

cd ~/klipper ; git fetch ; git checkout origin/work-lpcuart-20210504 ; sudo service klipper stop ; make ; make flash ; sudo service klipper start

-Kevin

Tested with skr 1.4T and it's working well

@KevinOConnor
Copy link
Collaborator

Thanks. I committed that change to the master branch (commit 45cd354).

-Kevin

@pmlody
Copy link

pmlody commented May 5, 2021

I can also confirm working set SKR 1.4 turbo with RPi Zero W

@KevinOConnor KevinOConnor added the resolved Issue is thought to now be fixed label May 6, 2021
@github-actions
Copy link

This ticket is being closed because the underlying issue is now thought to be resolved.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug resolved Issue is thought to now be fixed
Projects
None yet
Development

No branches or pull requests