Skip to content

Add RemRam v1 board variant #287

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

Merged
merged 4 commits into from
Sep 14, 2018
Merged

Add RemRam v1 board variant #287

merged 4 commits into from
Sep 14, 2018

Conversation

hasenbanck
Copy link
Contributor

RemRam is an open-source 3D printing controller.

https://github.com/hasenbanck/remram

RemRam is an open-source  3D printing controller.
It's source files can be found at [1].

[1] https://github.com/hasenbanck/remram
@fpistm fpistm self-requested a review July 27, 2018 16:45
@fpistm fpistm added the new variant Add support of new bard label Jul 27, 2018
@fpistm
Copy link
Member

fpistm commented Jul 27, 2018

Thanks @hasenbanck .

@hasenbanck
Copy link
Contributor Author

Don't merge yet. I might need to change timer configuration / IWDG. Will report back when I'm 100% sure.

Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

First review.
It seems your a not on top of the master. You should rebase on master.

PC3, // D68 - FAN_SPEED2

// Duplicated pins in order to be aligned with PinMap_ADC
PC0_2, //D69/A0 - THERM_1
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to duplicate the pins as they are grouped

PA_4, // D67 - FAN_SPEED1
PC_3, // D68 - FAN_SPEED2

// Duplicated pins in order to be aligned with PinMap_ADC
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to duplicate the pins as they are grouped

/* #define HAL_SPDIFRX_MODULE_ENABLED */
#define HAL_SPI_MODULE_ENABLED
#define HAL_TIM_MODULE_ENABLED
#define HAL_UART_MODULE_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

You could comment it, it is defined by default by platform.txt. Define it raised a warning about redefinition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commenting UART out would break building in Eclipse / Sloeber. I guess it would be better to life with a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After fixing the Serial stuff in the board.txt the build works without defining the UART here. I will comment it out then.

// PIN definition
#define NUM_DIGITAL_PINS 74
#define NUM_ANALOG_INPUTS 5
#define NUM_ANALOG_FIRST 69
Copy link
Member

Choose a reason for hiding this comment

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

So first is 64 or PC0

#define PWM_MAX_DUTY_CYCLE 255

// On-board LED pin number
#define LED_YELLOW 21
Copy link
Member

Choose a reason for hiding this comment

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

Preferred use the pin name PD0

boards.txt Outdated
@@ -725,3 +772,31 @@ Maple.menu.opt.o3lto.build.flags.ldspecs=-flto
Maple.menu.opt.ogstd=Debug (-g)
Maple.menu.opt.ogstd.build.flags.optimize=-g -Og
Maple.menu.opt.ogstd.build.flags.ldspecs=

RemRam.menu.opt.osstd=Smallest (-Os default)
RemRam.menu.opt.osstd.build.flags.optimize=-Os
Copy link
Member

Choose a reason for hiding this comment

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

Remove those 2 lines, they are useless as this is the default in platform.txt

RemRam.menu.opt.osstd.build.flags.optimize=-Os
RemRam.menu.opt.osstd.build.flags.ldspecs=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're sure about that? Using Eclipse / Sloeber won't show me the "-Os" Option without LTO if I remove those two lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, I got confused with the line numbering. Now I understand.

boards.txt Outdated
@@ -532,6 +571,14 @@ Maple.menu.xserial.none.build.xSerial=-DHAL_UART_MODULE_ENABLED -DHWSERIAL_NONE
Maple.menu.xserial.disabled=Disabled (No Serial)
Maple.menu.xserial.disabled.build.xSerial=

RemRam.menu.xserial.generic=Generic Serial
Copy link
Member

Choose a reason for hiding this comment

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

It seems your a not on the latest core.
You should rebase those menu has been review on 1.3.0.

@fpistm
Copy link
Member

fpistm commented Jul 29, 2018

What about IWDG? I'm currently review the IWDG library #234

@hasenbanck
Copy link
Contributor Author

I'm pretty sure that this PR is rebased to current master. I tripple checked the parent of my last commit. Odd.

Will fix the issues you marked.

I will push aside the IWDG until you merged #234 and I have more time to test.

 * Removed not needed pin mapping
 * Use new serial menu items
 * Removed unneeded UART declaration
 * Removed unneeded optimization
 * Use PIN name instead of PIN number
PC15, // D54 - BTN_EN1
PC14, // D55 - BTN_EN2
PC14, // D54 - BTN_EN1
PC15, // D55 - BTN_EN2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sic!

@fpistm
Copy link
Member

fpistm commented Jul 29, 2018

Right you are on top. I've asked this as I saw the old Serial Menu.
I've pull this PR and it is ok.
I will try to add the IWDG this week.

@hasenbanck
Copy link
Contributor Author

Thank you very much.

It seems that somehow the old generated file lacked the heap
definition. I re-created the script with the newest version of
Cube MX and the newest F7 driver package.
@fpistm
Copy link
Member

fpistm commented Jul 31, 2018

FYI, I've reviewed IWDG, based on the PR, I will push an updated version generic for all STM32 series.

@hasenbanck
Copy link
Contributor Author

Good to know. I'm currently reviewing the whole design, since I'm chasing a weird data corruption bug when I'm reading SD over SPI. That's why I saw the missing heap definition.

@fpistm
Copy link
Member

fpistm commented Jul 31, 2018

Ok let me know when you will confident to this variant as I can't test it due to lack of board to test.

@hasenbanck
Copy link
Contributor Author

I'm pretty sure now that this variant doesn't need a hardware rework anymore. It's ready for merge.

@fpistm
Copy link
Member

fpistm commented Aug 3, 2018

Hi @hasenbanck,
I've made the PR for IWDG. See #292
Any comment are welcome. ;)

@hasenbanck
Copy link
Contributor Author

@fpistm I hope to be able to get my controller board fully operational in my printer this weekend. Hopefully I can give you feedback about the IWDG in action then.

The two external PWM pins for the servos got mixed up.
This commits restores the correct order.
@hasenbanck
Copy link
Contributor Author

While testing the bugged Marlin code for servo handling, I realized that I also mixed up two pin assignments in the variant definition. The last commit fixes this issue.

@fpistm
Copy link
Member

fpistm commented Sep 20, 2018

Hi @hasenbanck
Just one question is it any reason to not enable the HAL_RTC_MODULE_ENABLED for the remram?

@hasenbanck
Copy link
Contributor Author

It's not needed for my primary build target, the Marlin firmware. Marlin doesn't use any RTC functionality.

@fpistm
Copy link
Member

fpistm commented Sep 20, 2018

Did you see any drawback if I enable it per default?
Even if enabled while the STM32RTC.h is not include the binary will not embed it.

@hasenbanck
Copy link
Contributor Author

Yeah, there shouldn't be any drawbacks in enabling it by default. You can do that if you want to stay consistent with the other boards.

@hasenbanck hasenbanck deleted the remram-variant branch October 1, 2018 09:04
xC0000005 pushed a commit to xC0000005/Arduino_Core_STM32 that referenced this pull request Nov 27, 2018
Add RemRam v1 board variant

RemRam is an open-source  3D printing controller.
It's source files can be found at [1].

[1] https://github.com/hasenbanck/remram

Signed-off-by: Nils Hasenbanck <[email protected]>
benwaffle pushed a commit to benwaffle/Arduino_Core_STM32 that referenced this pull request Apr 10, 2019
Add RemRam v1 board variant

RemRam is an open-source  3D printing controller.
It's source files can be found at [1].

[1] https://github.com/hasenbanck/remram

Signed-off-by: Nils Hasenbanck <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new variant Add support of new bard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants