DxCore
DxCore copied to clipboard
SpenceKonde
Logic input modes that do pin init are broken (for IN0 and maybe IN2)
I was troubleshooting why the logic blocks were not seeming to work properly for me, and noticed that the bit pattern in the CTRLB register didn't line up with what I was expecting based on the datasheet. Specifically it contained 0x75 instead of my expected 0x55, connecting the ZCD to IN1 instead of the external I/O. On closer inspection it looks like the library is at fault when one uses:
-
in::input_pullup -
in::input -
in::input_no_pullup
it does not properly mask off the upper bits that were used to signal to the library to configure the pin. Therefore, when written to the CTRLB register it also smashes part of the IN1 bits above it.
It looks like either changing L463&464 to explicitly mask the values written or changing L436 to input = in::pin; will fix the issue. I suspect that changing L436 is the better choice because of the savings in explicit masking, although it would mean that inspecting the state of LogicN no longer matches what the user coded.
Changing my code to use in::pin and have an explicit pinMode call setting up the pins results in working hardware for me.
I also changed L436 to in::pin instead and that seems to have fixed it as well.
https://github.com/SpenceKonde/DxCore/blob/master/megaavr/libraries/Logic/src/Logic.cpp#L436 https://github.com/SpenceKonde/DxCore/blob/master/megaavr/libraries/Logic/src/Logic.cpp#L463
Oh krutz.
@MCUdude It does look like there is a legit problem here, we should make sure to solve in in a unified manner,.
Thanks for using the library; that's a good catch! I'll guess the code below would do the trick? I don't have any hardware I can test with at the moment, but can you verify that this resolves the issue @alexferro?
// Set inputs modes
block.LUTCTRLB = ((input1 & 0x0f) << CCL_INSEL1_gp) | ((input0 & 0x0f) << CCL_INSEL0_gp);
block.LUTCTRLC = ((input2 & 0x0f) << CCL_INSEL2_gp);
I suspect that changing L436 is the better choice because of the savings in explicit masking, although it would mean that inspecting the state of LogicN no longer matches what the user coded.
Even though it comes with an overhead, it is important that the variables aren't "secretly" modified by the library like modifying line 436 would. Changing lines 463 and 464 makes the most sense from a library developer's perspective.
I'll put a TODO in my calendar to test when I get to work next week. The hardware is at $day_job so I can't test it until then. But I agree that it looks like it should work as well.
Even though it comes with an overhead, it is important that the variables aren't "secretly" modified by the library like modifying line 436 would. Changing lines 463 and 464 makes the most sense from a library developer's perspective.
If this is a concern, then the assignment that does just that on line 436 looks to be a violation of that principle as well? Since if it was in::input_pullup it'll be in::input after that code runs.
I can confirm that when I change my code back to specifying in::input_pullup and apply the fix @MCUdude posted above, that LUTCTRLB has the expected 0x55 rather than 0x75 as it does w/o the fix.
I've just pushed a fix to the MegaCoreX repo, where the initial issue has been resolved, and the input variable isn't changed from in::input_pullup to in::input.
I've fixed the register masking issue and the issue where the state in in::inputN is preserved even if set to in::input_pullup.