Changes for einstart s#11219
Conversation
eltoozero
left a comment
eltoozero
left a comment
There was a problem hiding this comment.
Is this acceptable?
.gitignore
Outdated
There was a problem hiding this comment.
These entries should be removed.
There was a problem hiding this comment.
Right so I need to treat my branch just like the upstream, pull a second copy locally and apply my config to do an install but leave the branch itself intact, roger that.
What's the best way to revert this? Delete the .gitignore and replace my configs with the defaults?
There was a problem hiding this comment.
It's fine to just remove the lines and add a new commit. I'll rebase and squash your branch before it gets merged.
Marlin/Configuration.h
Outdated
There was a problem hiding this comment.
Please don't modify the base configurations.
Marlin/Configuration_adv.h
Outdated
There was a problem hiding this comment.
Please don't modify the base configurations.
Marlin/Configuration_adv.h
Outdated
There was a problem hiding this comment.
If this reversion matches the configs submitted under config/examples/Einstart-S then those are most likely based on out-of-date configs and will need to be rebuilt based on the current configs.
There was a problem hiding this comment.
As tabs are somewhat unpredictable, we use 2 spaces for indentation throughout Marlin. Recommend you convert all tabs to spaces in this file.
Marlin/src/lcd/ultralcd.cpp
Outdated
There was a problem hiding this comment.
It's more correct to have U8GLIB_SH1106_EINSTART activate NEWPANEL in Conditionals_LCD.h as with the other SH1106 options. The NEWPANEL conditional indicates a panel that uses digital buttons rather than a shift register.
There was a problem hiding this comment.
I wasn't sure, but this did get LCD functional until I moved to native fastio, now it's gone.
I'll try switching the invocation to Conditiontals_LCD.h and invoke NEWPANEL and see if that fixes things up.
Would it be more or less hassle to just add a conditional to one of the existing SH1106 u8g constructors? After looking at the other implementations they're the same but just don't have RES and DC defined.
Might be more hassle than it's worth, I'm not sure if there's bigger plans for the LCD code and I don't want to make a mess of things.
There was a problem hiding this comment.
I think it's fine if this is separate from the other SH1106 items. If anything, I'd say just move it up next to one of those so they're grouped together.
There was a problem hiding this comment.
I moved it up with the rest, now I just need to establish what is the correct combo of LCD options to invoke to make it work.
From what I gather, the LCD defines in Configuration.h are picked up by Conditionals_LCD.h to enable the various DOGLCD, ULTIPANEL, NEWPANEL options, which are then compiled in via ultralcd.cpp and ultralcd_impl_DOGM.h.
In my initial brute force attack I got it working by adding a selectors at:
- Conditionals_LCD.h:166 (enabling ULTRA_LCD and DOGLCD)
- Conditionals_LCD.h:194 (enabling ULTIPANEL)
- ultralcd.cpp:4997 (set SET_INPUT(BTN_##)...)
- ultralcd_impl_DOGM.h:184 (actual u8g constructor)
Now I've got a single selector activating all the options in Conditionals_LCD:208 (ULTRA_LCD, DOGLCD, ULTIPANEL, NEWPANEL), no changes in ultralcd.cpp, and the constructor in place in ultralcd_impl_DOGM.h. No LCD.
I removed the if from ultralcd.cpp:4997 since by enabling NEWPANEL in Conditionals_LCD.h I should get the same code path, and I'm getting a large enough compile that I'm confident the includes are happening, but something isn't happy.
Marlin/src/pins/pins_EINSTART-S.h
Outdated
There was a problem hiding this comment.
There's no need for any of the #undef lines in this file.
Marlin/Configuration.h
Outdated
|
Typo somewhere in |
eltoozero
left a comment
eltoozero
left a comment
There was a problem hiding this comment.
I still need some assist getting LCD back and Menus functional, I had LCD status but no Menus prior to abandoning custom fastio/HAL. As a sanity check I still get u8glogo test sketch loaded ok using the same constructor but with my "naked" AVR variant and adjusted pins, M43 shows the pinout checks out, so not sure what's preventing LCD init.
|
So I did a sanity check test by uploading u8glogo to the board, and with stock settings I get nothing since D/C and RST are on PE2 and PE6, which are blocked in the IDE, we know this. I switch variants to my nude variant, changing the pins in the sketch to match chip pins, and I get logo, ok, sanity achieved. I make a fresh variant copying only the extra pins, exactly as they are configured in marlin pinsDebug_plus_70.h, now the pin definitions and constructor in u8glogo sketch are exactly as they are in pins_EINSTART-S.h; I upload u8glogo and it's happy. So, putting it all together, I upload marlin using the new variant, and I get my LCD back, and menus kind of work...I just need to tweak the buttons, left and right scroll up and down, but it works. I believe this confirms that u8glib is not using fastio, thus pins defined for the LCD in Configuration.h pass through unchanged to u8g, so if Arduino IDE doesn't acknowledge pin 78/79 then even though M43 debug will show correct assignments (via fastio), fastio isn't used for LCD communication... So either fork u8g to implement fastio (work) or use a variant in the IDE that supports then required pins; anecdotally u8glib is the only real library dependency in marlin I've ever run into. So what's the cleanest way forward, just make notes in the readme and put the pins_arduino.h in the examples folder? |
|
Anything else I can do to prep this for merge? I figure I’ll do another branch post-merge to get 9533 I2C LEDs working; this is 100% functional as it stands as long as a variant with blocked pins is used in the IDE to permit LCD functionality. Menu scrolling is awkward (left is down, right is up) but I’m pretty sure that’s a marlin bug and not a pins_.h issue on my part since the M43 definitions are correct. |
|
I went ahead and hacked away for the last few days and got the PCA9533 implemented; for our purposes it's equivalent to the PCA9632 even though there are only two PWM registers. I'm using one for red and one for blue so there's still the color change transition during heating, and green just takes an all or nothing value, so white works too (yay). Unfortunately that means orange doesn't work, but them's the breaks. So should I add the PCA9533 changes to this branch? Also, any feedback on cardinal directions and menu navigation re: right goes up and left goes down? For now I've just remapped things in pins_.h until I am corrected. |
Indeed the HAL does not need to be mucked around with to expose ATmega2560 pins not available as numbers on the MEGA board, I'll need to update the wiki with that tidbit and a reference to the pin-mapping comment in `fastio_1280.h`.
thinkyhead
force-pushed
the
changes-for-einstart-s
branch
from
12646fe to
7515ae5
Compare
I'll merge this first, and then you can submit a new PR later. |
2 participants
Added board to boards.h, added pins_EINSTART_S.h, made some trivial modifications to ultralcd.h and ultralcd_impl_DOGM.h to add a new SH1106 based constructor, it works.
This is a fresh copy without my modified HAL, Bob Kuhn set me straight on the pin declarations for non-exposed mega pins which are documented right at the top of in fastio_1280.h.