MarlinFirmware / Marlin

Marlin is an optimized firmware for RepRap 3D printers based on the Arduino platform. Many commercial 3D printers come with Marlin installed. Check with your vendor if you need source code for your specific machine.
https://marlinfw.org
GNU General Public License v3.0
16.13k stars 19.2k forks source link

NUM_SERVOS > 1 compile time error #7734

Closed hex4def6 closed 6 years ago

hex4def6 commented 6 years ago

Compile Error

When I compile with NUM_SERVOS > 1 I get an error:

sketch/servo.cpp: In member function 'void Servo::move(int)':
servo.cpp:312: error: static assertion failed: SERVO_DELAY must be an array NUM_SERVOS long.
   static_assert(COUNT(servo_delay) == NUM_SERVOS, "SERVO_DELAY must be an array NUM_SERVOS long.");
   ^
exit status 1
static assertion failed: SERVO_DELAY must be an array NUM_SERVOS long.

I believe commit e9c72978c71c92c996842d93683c895c3dc11787 is to blame; It added the following:

  constexpr uint16_t servo_delay[] = SERVO_DELAY;
  static_assert(COUNT(servo_delay) == NUM_SERVOS, "SERVO_DELAY must be an array NUM_SERVOS long.");

My C is rusty, but my understanding is that what is being asserted is not what was done to the array; shouldn't the array length be declared as NUM_SERVOS long if you're going to test against that?

I'm actually confused what this commit is trying to achieve; is the idea that each servo could have a separate delay (hence the array)? If so, there doesn't appear to be a way of setting them individually.

AnHardt commented 6 years ago

Extend the array until big enough for NUM_SERVOS

#define SERVO_DELAY { 300, 300 }
hex4def6 commented 6 years ago

Ah, of course. Thanks.

hex4def6 commented 6 years ago

I'd recommend placing something in the comment text & and/or assert message about that, something like "SERVO_DELAY should be comma separated list of values equal to NUM_SERVOS. For example, for a three servo configuration, you could use SERVO_DELAY { 300, 300, 300 }"

Roxy-3D commented 6 years ago

In C, if you don't specify the full number of initialization numbers... It should just replicate through the array.

int a[4] = {300};

should just fill the entire thing with 300's ????

hex4def6 commented 6 years ago

Well, with this in configuration.h:

#define NUM_SERVOS 2 // Servo index starts with 0 for M280 command

// Delay (in milliseconds) before the next move will start, to give the servo time to reach its target angle.
// 300ms is a good value but you can try less delay.
// If the servo can't reach the requested position, increase it.
#define SERVO_DELAY { 300 }

I get the issue described above. If I set servo_delay to:

#define SERVO_DELAY { 300, 300 }

It works.

Likewise, if I set NUM_SERVOS to 1, but leave SERVO_DELAY with { 300, 300} the assert fails (as it should).

So, I think it's working as advertised, it's just not very clear from the configuration.h comments that SERVO_DELAY is now an array.

DerAndere1 commented 5 years ago

It should indeed be explained in the comments of configuration.h that SERVO_DELAY is a braced init list for an array. Since C++ now has uniform initialization, that fact is not obvious from the syntax. @Roxy-3D : I had the same missunderstanding as @hex4def6 so I would not call this issue as resolved in marlin 2.0 bugfix.

DerAndere1 commented 5 years ago

Alternatively, change line 150 in Marlin/src/HAL/shared/servo.cpp from: constexpr uint16_t servo_delay[] = SERVO_DELAY; to the following: constexpr uint16_t servo_delay[NUM_SERVOS] = SERVO_DELAY;

Then, in configurations.h, SERVO_DELAY could be either a) a braced init list with 1 element like {300} OR (at the user's discretion) b) a braced list of n initializers (where n = NUM_SERVOS), e.g. {300, 300}

DerAndere1 commented 5 years ago

See also closed issues #7254, #7528, #8351

thinkyhead commented 5 years ago

Last I checked, elements that are left out in an initializer list get initialized to 0, not to the single provided value. So this:

constexpr uint16_t servo_delay[3] = { 300 };

…actually gives you this…

constexpr uint16_t servo_delay[3] = { 300, 0, 0 };
DerAndere1 commented 5 years ago

Oops. Anyway, once the documentation (e.g. at http://marlinfw.org/docs/configuration/configuration.html#extras-2 ) is updated after Marlin 2.0 is stable, this won't be an issue.

PS: Thanks for correcting me. I used too much Python recently. Marlin is great, I use it for my DIY lab robot (PipetBot-A8).

thinkyhead commented 5 years ago

Oy, yeah… The configuration documentation needs a major revision, and needs to be split up into several pages (most likely based on the @section directives in the files). It would also make sense to have a 1.1.9 version and a 2.0.x version because they have diverged a lot.

Sidans commented 5 years ago

I am new to these stuff . I have replaced the inductive auto leveling sensor with a BLTOUCH and following an instruction from you tube to configure BLTOUCH. the instruction says the number for NU_Servo should be changed to 1 from 2 ( my current setting ) can some one please explain to me what do we mean by NUM_Servo here. is it the number of servo motors on the printer? if yes, most printers have at least 3 motors. why 1 instead of 3? thanks

DerAndere1 commented 5 years ago

@Sidans regarding https://github.com/MarlinFirmware/Marlin/issues/7734#issuecomment-457964352. Welcome to the Marlin repository at github. It's good you looked for related issues before creating a new one. However you have a question regarding usage of Marlin and you obviously have not been observing the error discussed here. In future, try the following first, before using the issue tracker at github:

  1. read the fine manual (like for any other software/hardware)
  2. If you do not want to discuss a feature request or bug report:

    Do you want to ask a question? Are you looking for support? Please don't post here. Instead please use the Marlin Firmware forum at http://forums.reprap.org/list.php?415 or the Marlin Facebook Group https://www.facebook.com/groups/1049718498464482/.

  3. You can simply look for a [more related issue]: https://github.com/MarlinFirmware/Marlin/issues/5142.

After reading the very specific instructions for configurating Marlin1.1.x for BL Touch and the instructions for BL Touch found using google, you might realize that BL Touch is controlled in a similar way to an RC servo. (not to be confused with stepper motors).

github-actions[bot] commented 4 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.