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

✨ Add Dagoma D6 as found in DiscoUltimate v2 TMC #26874

Merged

Conversation

Sophist-UK
Copy link
Contributor

@Sophist-UK Sophist-UK commented Mar 15, 2024

Description

Add the Dagoma3D D6 motherboard as used in their Disco Ultimate v2 TMC printer (and possibly other models).

image

Requirements

Dagom3D D6 motherboard

Benefits

Adds support for a new motherboard.

Configurations

Taken from the Dagoma fork of Marlin DU_MC branch (where it is called FYSETC_DAGOMA_F5) and explicitly confirmed by Dagoma as being the D6 in response to a ticket I raised with them:

"the BOARD_FYSETC_DAGOMA_F5 is effectively the definition for the D6"

Related Issues

No.

Taken from the Dagoma fork of Marlin DU_MC branch where it is called FYSETC_DAGOMA_F5 and explicitly confirmed by Dagoma as being the D6:

"the BOARD_FYSETC_DAGOMA_F5 is effectively the definition for the D6"
@ellensp
Copy link
Contributor

ellensp commented Mar 15, 2024

env:mega2560 does not support io pins > 69

On This board [XYZE0]_SERIAL_RX_PIN and [XYZE0]_SERIAL_TX_PIN are set to IO pin 73.

for IO pins 70-85 you must used env:mega2560ext set in Marlin/src/pins/pins.h

@Sophist-UK
Copy link
Contributor Author

My mistake - I failed to copy the Dagoma comments. Pushing a fix now.

@InsanityAutomation
Copy link
Contributor

A dev working for Dagoma used to be active on discord a few years back. I sent him a ping to see if he was still active and/or employed there to chime in if there is anything to note.

@InsanityAutomation
Copy link
Contributor

Orel confirmed on discord that the naming was retained from a prototype board and everything matches, so just the env call to adjust.

@0r31
Copy link
Contributor

0r31 commented Apr 8, 2024

@ellensp and @InsanityAutomation : The board is based on the Fysetc F6, that's why we use pin 73.

@Sophist-UK : The right env to set is env:FYSETC_F6.

(Thanks @InsanityAutomation to ping me ;))

@0r31
Copy link
Contributor

0r31 commented Apr 8, 2024

Short explanation :
From platformio platform-atmelavr : https://github.com/platformio/platform-atmelavr/blob/d9d2738bcb1c632e2f5e3c57318e6ae8281ee960/boards/fysetc_f6_13.json#L13
There is a fysetcf6 variant.

I don't exactly know from where the pin definition file is retrieved (i don't find it https://github.com/arduino/ArduinoCore-avr), but in the platformio framework-arduino-avr package on my computer, the variant file exists : variants\fysetcf6\pins_arduino.h.

Inside this file, the pin 73 is explicity defined and used :
image

@InsanityAutomation
Copy link
Contributor

@0r31 It appears at a glance to be a limited implementation of what mega2560ext has, as it likely predates it, with just what they needed. Id like to see if it behaves with the standard definition in order to avoid confusion calling for a fysetc environment with a board not labeled fysetc, but if we cant confirm that then I suppose leaving it as-is works without any additional effort.

@0r31
Copy link
Contributor

0r31 commented Apr 8, 2024

@InsanityAutomation understood. I didn't know the fysetcf6 variant is "derived" from mega2560ext.

@Sophist-UK
Copy link
Contributor Author

I was a little bit concerned that this was going to end up being different from the Dagoma version, so happy that we have now settled on it being the same.

@thisiskeithb
Copy link
Member

@Sophist-UK: Please test this board with the mega2560ext environment and let us know if it still works as expected.

@InsanityAutomation
Copy link
Contributor

1 more hindsight thought here... The TMC slave addresses are sometimes in configs sometimes in pins... Should we move this one to the pins file to simplify things for end users?

@Sophist-UK
Copy link
Contributor Author

@thisiskeithb Sorry, but I don't have either the D6 board or the printer it is used in. I am trying to generate up to date versions of firmware for a variety of models equivalent to the out-of-date builds provided by the manufacturer.

But this is the board definition taken verbatim from the Marlin fork that they built their own firmware with.

@Sophist-UK
Copy link
Contributor Author

The TMC slave addresses are sometimes in configs sometimes in pins... Should we move this one to the pins file to simplify things for end users?

I don't know what this means - where would I find these TMC Slave Addresses defined in the Dagoma fork of Marlin for this board? In config_adv.h?

@thisiskeithb
Copy link
Member

thisiskeithb commented Apr 8, 2024

1 more hindsight thought here... The TMC slave addresses are sometimes in configs sometimes in pins... Should we move this one to the pins file to simplify things for end users?

We can assert them in the pins files like I did in #26842 so configs don’t get recycled into a new build with a different motherboard and end up with incorrect values.

@thisiskeithb
Copy link
Member

thisiskeithb commented Apr 8, 2024

But this is the board definition taken verbatim from the Marlin fork that they built their own firmware with.

Right, but we’re trying to generalize it so it’s not confusing for anyone looking at this in the future.

It's common for us to clean up a fork's implementation of code / pins / etc.

@0r31
Copy link
Contributor

0r31 commented Apr 8, 2024

@Sophist-UK @InsanityAutomation : The slave addresses are defined in the config file, as Marlins states it should be.

https://github.com/dagoma3d/Marlin/blob/DU_MC/Marlin/Configuration_adv.h#L2947-L2965

But to be printer agnostic, maybe it should be a good idea to add it in the pin definition file. I don't know which is the right convention to follow.

@InsanityAutomation
Copy link
Contributor

Theyre soldered on, so when the board is defined... immutable. Thats when I support setting them in pins file.

#define X_SLAVE_ADDRESS 0 #define Y_SLAVE_ADDRESS 1 #define Z_SLAVE_ADDRESS 2 #define E0_SLAVE_ADDRESS 3 #define E1_SLAVE_ADDRESS 3

Very often we surround em with an #ifndef to allow an override,

We have a very solid mix of which way theyre defined so I wouldnt say either is right or wrong.

@Sophist-UK
Copy link
Contributor Author

Sophist-UK commented Apr 8, 2024

I hope that I have done as requested by merging what @0r31 pointed in in the format that @thisiskeithb pointed to.

(Apologies for the delay in pushing - my PC is doing a large file xfer to my NAS :-( )

@0r31
Copy link
Contributor

0r31 commented Apr 8, 2024

@Sophist-UK : seems good to me.

I will test the board with the mega2560ext env before the end of the week. If it works, i will notify you then you could go to this env.

@Sophist-UK
Copy link
Contributor Author

I will test the board with the mega2560ext env before the end of the week.

Thank you - that is extremely good of you to offer.

@thisiskeithb thisiskeithb marked this pull request as draft April 8, 2024 17:35
@thisiskeithb
Copy link
Member

I will test the board with the mega2560ext env before the end of the week. If it works, i will notify you then you could go to this env.

Thank you! I've marked this PR as a draft for now until you can report back, so please let us know.

@0r31
Copy link
Contributor

0r31 commented Apr 9, 2024

Hi,

Just a quick update. Today i shortly checked :

  • The fmk arduino pin file related to mega2560ext ---> ok
  • The firmware compilation using this env ---> ok

Tomorrow ---> Tests

@0r31
Copy link
Contributor

0r31 commented Apr 10, 2024

@Sophist-UK,

It is a final go for me to use the mega2560ext env. The printer i use to test behaves exactly the same as before.

Thanks @InsanityAutomation and @thisiskeithb for your wise advices.

This reapplies the same fix suggested by early review comments.

Thanks to @0r31 for testing this. 👍
@Sophist-UK
Copy link
Contributor Author

Sophist-UK commented Apr 10, 2024

I have reapplied the env:mega2560ext fix suggested by early review comments.

Thanks to @0r31 for testing this. 👍

@thisiskeithb thisiskeithb marked this pull request as ready for review April 10, 2024 14:35
@thisiskeithb thisiskeithb changed the title Add Dagoma D6 board as used in their DiscoUltimate v2 TMC. ✨ Add Dagoma D6 as found in DiscoUltimate v2 TMC Apr 14, 2024
@sjasonsmith sjasonsmith merged commit 95d38a8 into MarlinFirmware:bugfix-2.1.x Apr 14, 2024
62 checks passed
@Sophist-UK
Copy link
Contributor Author

Thanks to everyone who helped with this PR - especially @0r31 Orel who has put in much more effort than I did.

@sjasonsmith
Copy link
Contributor

I observed that @0r31 did earlier work on this and jump in to test. I made sure they were credited as a co-author on the merge.

@Sophist-UK Sophist-UK deleted the define_dagoma_d6_mb branch April 15, 2024 15:37
blu28 added a commit to blu28/Marlin-blu that referenced this pull request Apr 20, 2024
commit 02ba6f9
Author: thinkyhead <[email protected]>
Date:   Fri Apr 19 00:21:25 2024 +0000

    [cron] Bump distribution date (2024-04-19)

commit dba0010
Author: Andrew <[email protected]>
Date:   Thu Apr 18 19:04:03 2024 -0400

    🎨 Rename some G-code files (MarlinFirmware#26981)

commit 90667f6
Author: I3DBeeTech <[email protected]>
Date:   Fri Apr 19 02:24:17 2024 +0530

    🐛 Fix BLACKBEEZMINI fan, info (MarlinFirmware#26983)

commit d6961b2
Author: thinkyhead <[email protected]>
Date:   Wed Apr 17 06:06:51 2024 +0000

    [cron] Bump distribution date (2024-04-17)

commit 07ebb81
Author: Javlon Sodikov <[email protected]>
Date:   Wed Apr 17 10:25:22 2024 +0500

    🩹Fix ProUI error when !CASELIGHT_USES_BRIGHTNESS (MarlinFirmware#26976)

    * Fix the compile error with the case light menu

    Fix the compile error with the case light menu

    * Add failing test

    ---------

    Co-authored-by: Jason Smith <[email protected]>

commit 245db73
Author: thinkyhead <[email protected]>
Date:   Tue Apr 16 18:06:16 2024 +0000

    [cron] Bump distribution date (2024-04-16)

commit 9342dae
Author: Scott Lahteine <[email protected]>
Date:   Tue Apr 16 12:17:47 2024 -0500

    📝 Remove dead PDF links

commit 1f84f50
Author: thinkyhead <[email protected]>
Date:   Mon Apr 15 02:38:10 2024 +0000

    [cron] Bump distribution date (2024-04-15)

commit 3326c74
Author: Scott Lahteine <[email protected]>
Date:   Sun Apr 14 16:26:16 2024 -0500

    📝 Minor README changes

commit 0269106
Author: Scott Lahteine <[email protected]>
Date:   Sun Apr 14 16:24:14 2024 -0500

    🎨 Dagoma D6 followup

commit 95d38a8
Author: Sophist <[email protected]>
Date:   Sun Apr 14 21:04:52 2024 +0100

    ✨ Add Dagoma D6 as found in DiscoUltimate v2 TMC (MarlinFirmware#26874)

    * Add Dagoma D6 board as used in their DiscoUltimate v2 TMC.

    Taken from the Dagoma fork of Marlin DU_MC branch where it is called FYSETC_DAGOMA_F5 and explicitly confirmed by Dagoma as being the D6:

    "the BOARD_FYSETC_DAGOMA_F5 is effectively the definition for the D6"

    ---------

    Co-authored-by: thisiskeithb <[email protected]>
    Co-authored-by: Orel <[email protected]>

commit dca6afc
Author: Chris <[email protected]>
Date:   Sun Apr 14 20:42:57 2024 +0200

    ✨🐛 HC32 - Add SERIAL_DMA, fix SDIO and MEATPACK (MarlinFirmware#26845)

    * fix meatpack on hc32

    * add support for SERIAL_DMA on HC32

    * add additional checks in HC32 HAL

    * migrate HC32 HAL to use app_config.h

    * fix memory leak in HC32 sdio HAL
    MarlinFirmware#26845 (comment)

    * hc32: fail if both EMERGENCY_PARSER and SERIAL_DMA are enabled

commit 19684f2
Author: Jason Smith <[email protected]>
Date:   Sat Apr 13 17:49:08 2024 -0700

    ✅ Add unit tests for macros.h (MarlinFirmware#26968)

commit 52a5613
Author: Keith Bennett <[email protected]>
Date:   Sat Apr 13 17:47:16 2024 -0700

    ⏪️ Revert unintended README changes (MarlinFirmware#26967)

    * Revert all the changes that went in with the unit test framework
    This restored broken links and other changes. Restoring to the prior revision seems the most appropriate action until the intentions of those file changes are known.
    ---------

    Co-authored-by: Jason Smith <[email protected]>

commit 0683e8a
Author: thinkyhead <[email protected]>
Date:   Sun Apr 14 00:24:15 2024 +0000

    [cron] Bump distribution date (2024-04-14)

commit 1bb4a04
Author: Jason Smith <[email protected]>
Date:   Sat Apr 13 14:11:51 2024 -0700

    ✅Unit test improvements (MarlinFirmware#26965)

    * Do not warn about display in unit tests

    * Treat warnings as errors in unit tests

    * Report actual filenames with unit tests

commit d10861e
Author: Jason Smith <[email protected]>
Date:   Sat Apr 13 12:06:08 2024 -0700

    ✅ Add unit testing framework (MarlinFirmware#26948)

    - Add a framework to build and execute unit tests for Marlin.
    - Enable unit test execution as part of PR checks.

    ---------

    Co-authored-by: Costas Basdekis <[email protected]>
    Co-authored-by: Scott Lahteine <[email protected]>

commit cf7c86d
Author: Andrew <[email protected]>
Date:   Sat Apr 13 14:59:59 2024 -0400

    🔧Fix M936 in features.ini (MarlinFirmware#26957)

commit d99e150
Author: David Buezas <[email protected]>
Date:   Sat Apr 13 18:54:25 2024 +0200

    ⚡️Reduce DISPLAY_SLEEP_MINUTES overhead (MarlinFirmware#26964)
RPGFabi pushed a commit to RPGFabi/Marlin that referenced this pull request Jun 15, 2024
* Add Dagoma D6 board as used in their DiscoUltimate v2 TMC.

Taken from the Dagoma fork of Marlin DU_MC branch where it is called FYSETC_DAGOMA_F5 and explicitly confirmed by Dagoma as being the D6:

"the BOARD_FYSETC_DAGOMA_F5 is effectively the definition for the D6"

---------

Co-authored-by: thisiskeithb <[email protected]>
Co-authored-by: Orel <[email protected]>
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.

6 participants