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

Improve MKS Robin support #19333

Merged
merged 46 commits into from
Jan 7, 2021

Conversation

Robin-DUBREUIL
Copy link
Contributor

@Robin-DUBREUIL Robin-DUBREUIL commented Sep 10, 2020

Updates on the MKS Robin pins configuration file

Hi everyone and thank you for working on the wonderful piece of software that Marlin is.
I managed to update pins_MKS_ROBIN.h to take advantage of most of the board capabilities.

Changes

  • Support for the 16Kb (2KB) onboard I2C EEPROM:tested working.
  • Support for the 64Kb (8KB) onboard SPI FLASH: not tested.
  • Support for the ColorUI interface on most MKS ROBIN TFT controllers (FSMC): tested working only with ILI9328 on MKS Robin TFT v1.1R.
  • SD now uses 4.5MHz SDIO (no card detect pin, needs edits to take adavantage of it on the MKS Robin V2.4): tested working on MKS Robin v2.2 and v2.3, latest bugfix required.
  • Added some hints on the required hardware mod to use SPI2 for additional devices.
  • Cleanup and refactoring.
  • Miscellaneous minor tweaks.

This code can still be improved as i do not use the LVGL interface because it lacks UBL and advanced pause feature so things may still be missing for the LVGL interface users. More work can be done to add or fix those features on MKS Robin derivatives, as i don't have those boards i prefer to let others improve their pin configuration file on the basis on my work.
Also, i've seen that a lot of work is being done to refactor the TFT interface (see #19192) and it sounds awesome, but it would need to update this file again.

### Updates on the MKS Robin pins configuration file
Hi everyone and thank you for working on the wonderful piece of software that Marlin is.
I managed to update pins_MKS_ROBIN.h to take advantage of most of the board capabilities.

### Changes
- Support for the 16Kb (2KB) onboard I2C EEPROM: **tested working.**
- Support for the 64Kb (8KB) onboard SPI FLASH: **not tested.**
- Support for the emulated DOGM interface on most MKS ROBIN TFT controllers (FSMC): **tested working only with ILI9328 on MKS Robin TFT v1.1R.**
- SD now uses 4.5MHz SDIO (no card detect pin, needs edits to take adavantage of it on the MKS Robin V2.4): **tested working on MKS Robin v2.2, latest bugfix required.**
- Added some hints on the required hardware mod to use SPI2 for additional devices.
- Cleanup and refactoring.
@Robin-DUBREUIL Robin-DUBREUIL marked this pull request as draft September 10, 2020 10:14
@thisiskeithb
Copy link
Member

thisiskeithb commented Sep 10, 2020

tested working on MKS Robin v2.2, latest bugfix required

Should some of these changes have been made to the Robin 2 pins instead? https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.0.x/Marlin/src/pins/stm32f4/pins_MKS_ROBIN2.h

Ignore me. These boards are difficult to keep track of when they all have the same model & numbers in the name.

@Robin-DUBREUIL
Copy link
Contributor Author

Hi,
MKS Robin and Robin 2 boards are completely different. The kind of board my PR deals with are the first Robin based on the STM32F103ZET6 MCU and it appears that mine is a v2.2. Robin 2 boards are newer and more powerful boards based on the STM32F407ZET6 MCU, so apparently none of the changes i made could possibly affect those boards (different HAL, pin mappings, features...).

@sjasonsmith
Copy link
Contributor

@thisiskeithb that is the naming confusion in mentioned the other day. Robin v2 != Robin2

@sjasonsmith
Copy link
Contributor

@mattdog01 does this seem related to the SDIO issues you’ve been having with a Robin? @TheCodeExorcist, which PlatformIO environment are you using this with? mks_robin or mks_robin_stm23?

@rhapsodyv
Copy link
Member

Also, i've seen that a lot of work is being done to refactor the TFT interface (see #19192) and it sounds awesome, but it would need to update this file again.

Sure, one of the idea from the TFT refactoring, is to remove the need of board know about the UI. The board will just setup FSMC / SPI interface, and it's done.
There's another place where the display types are defined, with their specs: driver, resolution, orientation, etc. The user just select the UI and the Display type.

@Robin-DUBREUIL
Copy link
Contributor Author

Robin-DUBREUIL commented Sep 10, 2020

@sjasonsmith, I built this on PlatformmIO for Code OSS withe the standard mks_robin environment. I suspect that the mks_robin_stm32 environment is kind of a relic from the past as i don't see any practical use case for it nowadays...
However, i forgot to mention that i edited W25Qxx.cpp to be able to use the ENABLED() macro here in pins_MKS_ROBIN.h:
#define HAS_SPI_FLASH #if ENABLED(HAS_SPI_FLASH)
So, you have to change those two lines this way in attempt to be able to compile marlin with a stock W25Qxx.cpp:
#define HAS_SPI_FLASH 1 #if HAS_SPI_FLASH

Nevermind, i just edited the file accordingly.

Reverting back changes to make file compilable with a stock W25Qxx.cpp file.
@sjasonsmith
Copy link
Contributor

mks_robin_stm32 is very new. Different overlapping efforts for TFT support resulted in it being first implemented for that environment, then @rhapsodyv later expanding it to support more environments.

@Robin-DUBREUIL
Copy link
Contributor Author

@sjasonsmith I better understand now where this environment comes from, i had some severe compilation issues with it this is why I pushed some efforts to get a functionnal ColorUI on the standard mks_robin environment, on the basis of what has been done on other boards and by reading some of the Marlin code.

@mattdog01
Copy link

@mattdog01 does this seem related to the SDIO issues you’ve been having with a Robin? @TheCodeExorcist, which PlatformIO environment are you using this with? mks_robin or mks_robin_stm23?

@sjasonsmith I suspect it does. I have the MKS-Robin v2.3 :)

FYI I ran a couple hour print last night with the latest bugfix available (last night) and it still had the random print head movements to the ends of the axis and back but with a new one, the extruder tried to drive a what seemed like 1000+mm of filament through the hot end in a couple seconds and of course just shredded the filament and then continued to the end of the print. It didn't reset but it did have issues reading the SD card. Sorry if that is too much info.

@Robin-DUBREUIL
Copy link
Contributor Author

@mattdog01 I compiled my firmware last night too with the latest bugfix available and my custom config file. My printer worked like a charm all night long so you should definitely give it a try. I precised that the latest bugfix is needed only because of this fix recently made for boards without SD detect pin: #19236

@mattdog01
Copy link

@TheCodeExorcist I'm not a coder and know little about how the files should be / are arranged, so I will need to wait for it to get pulled into bugfix for me to get your fixes. How long does that normally take?
I looked at the piece of code you have on the other issue. Does that just need to be pasted into the pins_mks_robin file? I'm assuming at that location?
Any config setting changes? Build in mks_robin_stm32?

@mattdog01
Copy link

@TheCodeExorcist I could't find any of this text in my pins_MKS_ROBIN.h in order the know where to change the code there. Best if I just look and not touch.

W25Q64 64Mb (8MB) SPI flash
#define HAS_SPI_FLASH

@Robin-DUBREUIL
Copy link
Contributor Author

Robin-DUBREUIL commented Sep 10, 2020

@mattdog01, many things have been added over the original pins_MKS_ROBIN.h so the easiest way to go is to simply replace the original file by my modified one. You have to build with the mks_robin environment as i did not achieve to build it with mks_robin_stm32 and as i don't see any advantage of using mks_robin_stm32 over mks_robin i don't think i will try to make it work. Your config should remain the same as long as you use FSMC_GRAPHICAL_TFT (for upscaled Marlin UI) or TFT_320x240 (for Color UI) and if you don't, switch to one. If you choose TFT_320x240 to take advantage of Color UI, you may get a non working screen and if that happens, try commenting this line #define TFT_ILI9328 and decommenting the next driver //#define TFT_R61505 and so on until you find the driver your TFT uses...

@sjasonsmith
Copy link
Contributor

i don't see any advantage of using mks_robin_stm32 over mks_robin

The hope is to eventually move everything from HAL/STM32F1 to HAL/STM32, making mks_robin_stm32 the future. That said, there is no immediate timeline for that migration. PlatformIO has deprecated the Maple framework used by HAL/STM32F1, and from a maintenance perspective there are advantages to only maintaining a single STM32 HAL.

@mattdog01 is having some reliability issues with his SD reads using mks_robin_stm32, so this will be a good data point to determine whether the same problems exist when he is using your version with the mks_robin environment.

@Robin-DUBREUIL
Copy link
Contributor Author

Robin-DUBREUIL commented Sep 10, 2020

@sjasonsmith Ok, so with that said every STM32 based board should move to the unified STM32 HAL with no real deadline for now. Is the MKS Robin use of the unified STM32 HAL a standard way of doing it (all boards should do the same way) or something still hacky ?
It is in fact interesting to see if @mattdog01 have the same issues on mks_robin because this new HAL seems to be in use for a few boards only so we have less feedback on running marlin with it. I think this should still be considered unsafe for production, right ?

Marlin/src/pins/stm32f1/pins_MKS_ROBIN.h Outdated Show resolved Hide resolved
Comment on lines 122 to 139
#ifdef ARDUINO_ARCH_STM32F1
#define BEEPER_PIN PC13
#else
#define BEEPER_PIN -1
#endif
//
// Piezzoelectric speaker
//
#define BEEPER_PIN PC13
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this was -1 when using the STM32 HAL?

This might introduce a timer conflict for mks_robin_stm32, but I'm not too worried about that. If that happens a little extra work will be needed to possibly move the TONE_TIMER, but that would be the right thing to do.

I verified that PC13 isn't attached to any timer pins. The Maple framework will take over any timer attached to BEEPER_PIN if SPEAKER is enabled, which can make enabling it risky for some boards. It seems ok for this board.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The beeper works fine when compiled with mks_robin on my board, i should try compiling with mks_robin_stm32 to see if everything is still fine but for now i have other issues to solve to make marlin compile on this framework with my config...

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this was -1 when using the STM32 HAL?

There was a bug that caused Marlin with HAL STM32 hangs when speaker enabled.
I have not tested this for a while, so I can't tell if this bug still present.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you tested before I upgraded to ststm32 8.0 that might be fixed. I think I remember TONE not working on one timers, but it worked with the newer framework.

Copy link
Member

Choose a reason for hiding this comment

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

The only problem in BEEPER_PIN that I know is when SPEAKER is enabled. I'm using the BEEPER in a STM32F4, on HAL STM32 with no problem, in the current version.

Marlin/src/pins/stm32f1/pins_MKS_ROBIN.h Outdated Show resolved Hide resolved
@sjasonsmith
Copy link
Contributor

@TheCodeExorcist I only just discovered that mks_robin_stm32 environment. It slipped in with a TFT PR, so I didn't notice it happened (I tend to ignore display-related work).

I haven't really looked at it close enough to have an opinion on it. I would not assume it is perfect, especially with one of the few early users reporting SD reliability issues.

Nobody has really formulated a plan for moving things over. I would prefer an approach where we get one or two boards really stable and well-defined, which then serves as a template for the rest. The real test will be moving the SKR E3 Mini over, to see if a week can go by without a ton of new issues reported!

commenting TMC SOFTWARE SERIAL by default
@Robin-DUBREUIL
Copy link
Contributor Author

Robin-DUBREUIL commented Sep 10, 2020

Nobody has really formulated a plan for moving things over. I would prefer an approach where we get one or two boards really stable and well-defined, which then serves as a template for the rest. The real test will be moving the SKR E3 Mini over, to see if a week can go by without a ton of new issues reported!

This would be indeed a quite fireproof test, i'll try to see what i can do on the Robin side.

@sjasonsmith
Copy link
Contributor

In the meantime, it is fine to continue fixing things in the STM32F1 versions, I just wouldn't recommend large development investments in it.

@mattdog01
Copy link

@TheCodeExorcist So far so good. I went to your repo copied and pasted the contents of pins_MKS_ROBIN.h to my pins_MKS_ROBIN.h . It compiled just fine, The new UI looks good. SD card was ready to be used, no need to use the custom commands to get it working.
@sjasonsmith I think I will need to specify a different set of servo pins and move my servo wires to that location because my servo for bed leveling doesn't work anymore.

@mattdog01
Copy link

@sjasonsmith No, way easier than that. Just move the connector to the next servo connection on the board. Works great.

…_patch-1

# Conflicts:
#	Marlin/src/pins/stm32f1/pins_MKS_ROBIN.h
@sjasonsmith
Copy link
Contributor

I have rebased this on the current state of bugfix-2.0.x. I guarantee I have made mistakes. @rhapsodyv is going to review the code, the TFT and SD definitions in particular.

I did not force push this. I rebased, then merged the remote branch into my rebase. This means my rebase is reversible if I made a horrible mess of things.

@sjasonsmith sjasonsmith added the Needs: Work More work is needed label Dec 12, 2020
@sjasonsmith
Copy link
Contributor

@rhapsodyv I don't know how to configure TFT displays. I left the various display sections in the file, for you to review. I have no idea whether these are still necessary or are correct.

@thinkyhead thinkyhead requested a review from rhapsodyv December 22, 2020 13:43
@sjasonsmith
Copy link
Contributor

I resolved merge conflicts on this again. I think it can only move forward if @rhapsodyv or @TheCodeExorcist respond and provide feedback. I had to do a lot of "educating guessing" when I updated this 20 days ago.

@rhapsodyv
Copy link
Member

@mattdog01 are you using the pins from here? or from bugfix?

Comment on lines 56 to 68
/**
* STM32F1 APB2 = 72MHz, APB1 = 36MHz, max SPI speed of this MCU if 18Mhz
* STM32F1 has 3 SPI ports, SPI1 in APB2, SPI2/SPI3 in APB1
* so the minimum prescale of SPI1 is DIV4, SPI2/SPI3 is DIV2
*/
#if SPI_DEVICE == 1
#define SPI_CLOCK_MAX SPI_CLOCK_DIV4
#else
#define SPI_CLOCK_MAX SPI_CLOCK_DIV2
#endif
uint8_t clock;
switch (spiRate) {
case SPI_FULL_SPEED: clock = SPI_CLOCK_MAX; break;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems like it does not belong with this PR. @rhapsodyv do you know anything about this?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. In fact we can just keep as it is today. I'm changing those spi calls anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Keep it as it is here today? Or keep it as it is in bugfix today, before these changes?

@mattdog01
Copy link

@mattdog01 are you using the pins from here? or from bugfix?

@rhapsodyv I was using the bugfix files without any changes except in the configuration files.

@thinkyhead thinkyhead merged commit 3009707 into MarlinFirmware:bugfix-2.0.x Jan 7, 2021
@thinkyhead thinkyhead changed the title Update pins_MKS_ROBIN.h Improve MKS Robin support Jan 7, 2021
@ETE-Design
Copy link
Contributor

@TheCodeExorcist You write "SD now uses 4.5MHz SDIO (no card detect pin, needs edits to take adavantage of it on the MKS Robin V2.4): tested working on MKS Robin v2.2 and v2.3, latest bugfix required." I have an MKS Robin Lite who dosen't seem to have card pin, what needed to be changed to use it with that board if possible?

@Robin-DUBREUIL
Copy link
Contributor Author

Robin-DUBREUIL commented Jan 7, 2021 via email

@ETE-Design
Copy link
Contributor

@TheCodeExorcist Thanks for taking your time to answer me thougt the PR is closed... Mabye you can help me a little?
How to find/know #define SDIO_CLOCK and #define SDIO_READ_RETRIES
Also what is WIFI_IO0_PIN for?

@Robin-DUBREUIL
Copy link
Contributor Author

Robin-DUBREUIL commented Jan 7, 2021 via email

susisstrolch pushed a commit to susisstrolch/Marlin that referenced this pull request Jan 8, 2021
… into bugfix-2.0.x

* 'bugfix-2.0.x' of https://github.com/MarlinFirmware/Marlin: (105 commits)
  [cron] Bump distribution date (2021-01-08)
  Fix M48 output (MarlinFirmware#20713)
  Improved MKS Robin support (MarlinFirmware#19333)
  Preheat before Power Loss Recovery homing (MarlinFirmware#20697)
  [cron] Bump distribution date (2021-01-07)
  Custom build_flags by feature (MarlinFirmware#20692)
  [cron] Bump distribution date (2021-01-06)
  Multi-Z stepper inverting (MarlinFirmware#20678)
  Fix Azteeg X3 macro typo (MarlinFirmware#20681)
  Define SANGUINOLOLU 1.1 enable pins (MarlinFirmware#20682)
  No BTN_ENC_EN on Anet 10 (MarlinFirmware#20684)
  Temperature report followup (MarlinFirmware#20687)
  Adjustable precision in M105 temperature report (MarlinFirmware#20602)
  Don't apply hotend_offset.z to Z soft endstops (MarlinFirmware#20675)
  Indent tool_change_prime
  Clarify solenoid active / magnet-on state
  Defer "quiet probing" till the last Z bump (MarlinFirmware#20610)
  Solenoid cleanups
  [cron] Bump distribution date (2021-01-05)
  Remove untranslated strings
  ...
dpreed pushed a commit to dpreed/Marlin_2.0.x that referenced this pull request Feb 5, 2021
kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021
W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021
thinkyhead pushed a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021
thinkyhead pushed a commit that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants