Henning/vtd multi agent

[clssn]

This feature branch enables Cloe's VTD binding to perform multi-agent simulation.
Attention: We need to first finish the ben/add-vtd-package PR and then rebase this one here onto the resulting master.

---

This change is 

[cassava]

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @clssn)

---

plugins/vtd/src/vtd_binding.cpp, line 206 at r3 (raw file):

      // Wait for creation of TaskControl client
      logger()->info("Wait for task control...");
      scp_try_read_until([this]() { return static_cast<bool>(task_control_); });

return task_control_ != nullptr;

---

plugins/vtd/src/vtd_binding.cpp, line 211 at r3 (raw file):

      if (!config_.scenario.size()) {
        logger()->info("Wait for scenario...");
        scp_try_read_until([this]() { return config_.scenario.size(); });

I would say use scenario != "" since the intention is clearer.
Otherwise I think it's best practice to not use 0 == false shortcuts and write it out explicitely for those things...

---

plugins/vtd/src/vtd_binding.cpp, line 212 at r3 (raw file):

        logger()->info("Wait for scenario...");
        scp_try_read_until([this]() { return config_.scenario.size(); });
        // Stop to neutralize GUI's Init command sendt along with LoadScenario

Typo: "sent"

---

plugins/vtd/src/vtd_binding.cpp, line 220 at r3 (raw file):

      query.scenario = config_.scenario;
      scp_client_->send(query);
      scp_try_read_until([this]() { return !agents_expected_.empty(); });

We should note for all these scp_try_read_until function calls note where we expect the predicate to be set to true.

---

plugins/vtd/src/vtd_binding.cpp, line 464 at r3 (raw file):

   */
  void scp_try_read_until(std::function<bool(void)> pred) {
    int tries_left = VTD_INIT_WAIT_RETRIES;

I thought VtdConf had a value for this...

---

plugins/vtd/src/vtd_binding.cpp, line 616 at r3 (raw file):

    if (p.is_absolute()) {
      // make relative to first subdirectory called "Scenarios"
      auto s = std::find(p.begin(), p.end(), "Scenarios");

Print a warning if the path has more than one "Scenarios" element, letting the user know why this might be a problem.

[clssn]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @cassava and @clssn)

---

plugins/vtd/src/vtd_binding.cpp, line 464 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

I thought VtdConf had a value for this...

We have individual TcpTransceiverConfiguration used for connecting to scp, paramserver, taskcontrol and the sensors. But there we want to sleep longer between the retries. It would stall initialization if we'd wait for seconds here. I propose to leave the sleep duration a constant and re-use the number of retries from the scp transceiver's config.

[cassava]

---

plugins/vtd/src/vtd_binding.cpp, line 464 at r3 (raw file):

Previously, clssn (Henning Claßen) wrote…

We have individual TcpTransceiverConfiguration used for connecting to scp, paramserver, taskcontrol and the sensors. But there we want to sleep longer between the retries. It would stall initialization if we'd wait for seconds here. I propose to leave the sleep duration a constant and re-use the number of retries from the scp transceiver's config.

Hm, sure. We can always add a configuration value later if we need it.

[clssn]

Reviewable status: 7 of 11 files reviewed, 6 unresolved discussions (waiting on @cassava)

---

plugins/vtd/src/vtd_binding.cpp, line 206 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

return task_control_ != nullptr;

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 211 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

I would say use scenario != "" since the intention is clearer.
Otherwise I think it's best practice to not use 0 == false shortcuts and write it out explicitely for those things...

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 212 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

Typo: "sent"

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 220 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

We should note for all these scp_try_read_until function calls note where we expect the predicate to be set to true.

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 464 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

Hm, sure. We can always add a configuration value later if we need it.

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 616 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

Print a warning if the path has more than one "Scenarios" element, letting the user know why this might be a problem.

Done.

[cassava]

---

plugins/vtd/src/vtd_binding.cpp, line 212 at r5 (raw file):

      if (!config_.scenario.size()) {
        logger()->info("Wait for scenario...");
        // Expect the scenario to be initialized in VtdBiniding::apply_scenario_filename()

Typo: VtdBiniding

[clssn]

---

plugins/vtd/src/vtd_binding.cpp, line 212 at r5 (raw file):

Previously, cassava (Ben Morgan) wrote…

Typo: VtdBiniding

Done.

[cassava]

---

plugins/vtd/src/vtd_binding.cpp, line 616 at r3 (raw file):

Previously, clssn (Henning Claßen) wrote…

Done.

You missed this one.

[cassava]

Reviewed 2 of 3 files at r4, 2 of 2 files at r5.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @cassava and @clssn)

[cassava]

Reviewed 1 of 1 files at r7.
Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @cassava and @clssn)

[cassava]

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @clssn)

[cassava]

---

plugins/vtd/src/vtd_binding.cpp, line 629 at r8 (raw file):

      }
      fs::path r;
      for (auto it = ++s; it != p.end(); ++it) r /= *it;

Missing { brackets } around content of for-loop :-(

[cassava]

---

plugins/vtd/src/vtd_binding.cpp, line 632 at r8 (raw file):

      if (std::find(s, p.end(), "Scenarios") != p.end()) {
        logger()->warn(
            "Cannot determine the scenario directory unambiguously because"

This is not going to look great printed...
"unambiguously becausethe chosen"

[cassava]

Reviewed 1 of 2 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @clssn)

[clssn]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @cassava)

---

plugins/vtd/src/vtd_binding.cpp, line 616 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

You missed this one.

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 629 at r8 (raw file):

Previously, cassava (Ben Morgan) wrote…

Missing { brackets } around content of for-loop :-(

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 632 at r8 (raw file):

Previously, cassava (Ben Morgan) wrote…

This is not going to look great printed...
"unambiguously becausethe chosen"

Done.

[cassava]

Reviewed 1 of 1 files at r9.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[clssn]

Reviewed 1 of 2 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[cassava]

Goood catch!

[cassava]

Reviewed 1 of 1 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[clssn]

Reviewed 1 of 1 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[cassava]

Reviewed 1 of 1 files at r12.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[tobifalk]

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 3 of 4 files at r3, 2 of 3 files at r4, 1 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @clssn)

---

plugins/vtd/src/vtd_binding.cpp, line 211 at r3 (raw file):

Previously, clssn (Henning Claßen) wrote…

Done.

There are a couple more config_.scenario.size() in this file. Should those be changed as well?

---

plugins/vtd/src/vtd_binding.cpp, line 324 at r6 (raw file):

    // If in reset, block until VTD sends "Run" again then start next cycle
    while (!operational_) {

Should we take this chance to replace operational_ by is_operational()?

[cassava]

---

plugins/vtd/src/vtd_binding.cpp, line 324 at r6 (raw file):

Previously, tobifalk (Tobias Falkenstein) wrote…

Should we take this chance to replace operational_ by is_operational()?

I'd rather do it the other way around. Because is_operational() is an interface method and probably shouldn't be used internally for more than it externally promises...
Also operational_ is easy to search for where it is changed.

[clssn]

---

plugins/vtd/src/vtd_binding.cpp, line 211 at r3 (raw file):

Previously, tobifalk (Tobias Falkenstein) wrote…

There are a couple more config_.scenario.size() in this file. Should those be changed as well?

Yes, definitely. Thanks!

[cassava]

---

plugins/vtd/src/vtd_binding.cpp, line 211 at r3 (raw file):

Previously, clssn (Henning Claßen) wrote…

Yes, definitely. Thanks!

Yes please? There's also empty() isn't there?

[clssn]

---

plugins/vtd/src/vtd_binding.cpp, line 324 at r6 (raw file):

Previously, cassava (Ben Morgan) wrote…

I'd rather do it the other way around. Because is_operational() is an interface method and probably shouldn't be used internally for more than it externally promises...
Also operational_ is easy to search for where it is changed.

Agreed.

[cassava]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @clssn)

[clssn]

---

plugins/vtd/src/vtd_binding.cpp, line 211 at r3 (raw file):

Previously, cassava (Ben Morgan) wrote…

Yes please? There's also empty() isn't there?

No empty() in strings. Only size().

[clssn]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cassava and @tobifalk)

---

plugins/vtd/src/vtd_binding.cpp, line 211 at r3 (raw file):

Previously, clssn (Henning Claßen) wrote…

No empty() in strings. Only size().

Done.

---

plugins/vtd/src/vtd_binding.cpp, line 324 at r6 (raw file):

Previously, clssn (Henning Claßen) wrote…

Agreed.

Done.

[tobifalk]

Reviewed 1 of 1 files at r13.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cassava and @tobifalk)

[cassava]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tobifalk)

[tobifalk]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[tobifalk]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[cassava]

Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @cassava)

[tobifalk]

Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @cassava)

[cassava]

Reviewed 1 of 1 files at r13.
Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @cassava)

[cassava]

Reviewed 1 of 1 files at r15.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[clssn]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[cassava]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)

[tobifalk]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @clssn)