Skip to content

Enhancements to M43 pins debugging#5118

Merged
thinkyhead merged 9 commits intoMarlinFirmware:RCBugFixfrom
thinkyhead:rc_expanded_M43
Oct 31, 2016
Merged

Enhancements to M43 pins debugging#5118
thinkyhead merged 9 commits intoMarlinFirmware:RCBugFixfrom
thinkyhead:rc_expanded_M43

Conversation

@thinkyhead
Copy link
Member

Cleanup and optimization of #5112

  • Add endstop monitoring option with E parameter
  • Report PWM status of pins
  • Give an extended pin report when not watching with W

Copy link
Contributor

@Blue-Marlin Blue-Marlin Oct 31, 2016

Choose a reason for hiding this comment

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

The pins_arduino.h files provide this info in NUM_DIGITAL_PINS and NUM_ANALOG_INPUTS

Copy link
Contributor

Choose a reason for hiding this comment

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

endstop_monitor_flag could be checked here and would save the call of endstop_monitor() if not active.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the tip. I totally forgot to review that part of the code.

@thinkyhead thinkyhead force-pushed the rc_expanded_M43 branch 3 times, most recently from 8103435 to a092edb Compare October 31, 2016 13:05
@thinkyhead thinkyhead merged commit fa6bf12 into MarlinFirmware:RCBugFix Oct 31, 2016
@thinkyhead thinkyhead deleted the rc_expanded_M43 branch October 31, 2016 13:29
@Bob-the-Kuhn
Copy link
Contributor

Much cleaner code

I've got a couple of bug fixes & a couple of minor changes.

All the changes are included in the attached file.
pinsDebug .zip

I couldn't get the #define PINSET(P) (defined(P) && P > -1) from the previous review to work on a pin that existed. Really strange since it uses the same method as the PIN_EXISTS macro.

When determining if a pin is connected to a PWM, the TCCRxA register is always used even if looking at TIMERxC

Lines 677-680 need to change

#define PWM_CASE(N) \
  case TIMER##N: \
    if (TCCR##N & (_BV(COM## N ##1) | _BV(COM## N ##0))) { \
      PWM_PRINT(OCR##N); \

The solution I came up with is a two part change:
Lines 677-680 now read

#define PWM_CASE(N,Z) \
  case TIMER##N##Z: \
    if (TCCR##N ##A & (_BV(COM## N ## Z ##1) | _BV(COM## N ## Z ##0))) { \
      PWM_PRINT(OCR##N ## Z); \

which means 694-724 change from
PWM_CASE(0A);
to
PWM_CASE(0,A);

In the WGM_MAKE macros the TEST function only returns 1 or 0 so the following pieces in lines 734-735 need to change from

TEST(TCCR##N##B, WGM##N##2) >> 1
TEST(TCCR##N##B, WGM##N##3) >> 1

to either

TEST(TCCR##N##B, WGM##N##2) << 2
TEST(TCCR##N##B, WGM##N##3) << 3

or

TCCR##N##B & _BV(WGM##N##2)) >> 1
TCCR##N##B & _BV(WGM##N##3)) >> 1

I prefer the latter because it's one less operation to perform and it's consistent with how the macros treat the other register.

A couple of minor changes:

  • buffer[30] changed to buffer[40] per review in previous PR
  • added name length to comment on line 75
    // Pin list updated from 7 OCT RCBugfix branch - max length of pin name is 24

@thinkyhead
Copy link
Member Author

Really strange since it uses the same method as the PIN_EXISTS macro.

Almost. When PIN_EXISTS tries to evaluate an undefined symbol the result is:

defined(_PIN) && _PIN >= 0

…but with the proposed PINSET it ends up being:

defined() && >= 0

…which is nonsense. I hit this before, but I forgot.

@thinkyhead
Copy link
Member Author

thinkyhead commented Oct 31, 2016

Ah, you're right about my mis-use of TEST. I forgot it simply evaluates as 1 or 0, and not as the bit.

@thinkyhead
Copy link
Member Author

thinkyhead commented Oct 31, 2016

buffer[30] changed to buffer[40] per review in previous PR

I think 30 is fine. I forgot that justified strings already include the string length.

thinkyhead pushed a commit to thinkyhead/Marlin that referenced this pull request Nov 1, 2016
thinkyhead added a commit that referenced this pull request Nov 1, 2016
Followup to #5118 - pins debugging cleanup
@thinkyhead thinkyhead mentioned this pull request Nov 1, 2016
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.

3 participants