Skip to content

Configurable ROV#276

Merged
cal-pratt merged 8 commits intomasterfrom
configurable-rov
May 1, 2017
Merged

Configurable ROV#276
cal-pratt merged 8 commits intomasterfrom
configurable-rov

Conversation

@ConnorWhalen
Copy link
Contributor

Addresses #270.

The file defaultconfig.yml contains the default configuration. To override the default config, create a new .yml file and set the configFile value in defaultConfig.yml to the name of the new config file. The overriding config does not need a value for every constant and if the file does not exist it carries on with the default.

@ConnorWhalen ConnorWhalen force-pushed the configurable-rov branch 2 times, most recently from 7948bd1 to 4d55c95 Compare March 18, 2017 19:42
Copy link
Contributor

@cal-pratt cal-pratt left a comment

Choose a reason for hiding this comment

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

Looking good so far. Main note right now is about getting rid of the existing command line options and replacing with config file locations. It will also be good to get some test coverage on the Config class you created.

You can probably get away with inlining a short yml file in java strings:

final File temp = File.createTempFile("config", ".yml");
try (final BufferedWriter writer = new BufferedWriter(new FileWriter(temp))) {
    writer.write("foo:\n   bar: 1\n");
} catch ... 

Or just create a file if that's easier, however, we'll then have to think about where we want to place non-java test-files.

build.gradle Outdated
}

dependencies {
compile group: "org.cfg4j", name:"cfg4j-core", version: "4.4.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

muh ocd plz write in format like the others

@@ -0,0 +1,25 @@
configFile: config.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking for later that we can change the main methods to accept the config files as program args. This def is okay here for now.

.startWith(new CameraSpeedValueA())
.cast(SpeedValue.class),
channels.get(CAMERA_A_MOTOR_CHANNEL).setOutputRange(new Range(Motor.MAX_REV, Motor.MAX_FWD))),
channels.get(config.cameraAMotorChannel()).setOutputRange(new Range(Motor.MAX_REV, Motor.MAX_FWD))),
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: start setOutputRange on new line to match thruster code.
vertical is easier to read than horizontal.

@@ -358,10 +326,12 @@ public static void main(final String[] args) throws InterruptedException, IOExce
final EventPublisher eventPublisher = new BroadcastEventPublisher(new UdpBroadcast<>(
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the Option broadcast = Option.builder... stuff should go. broadcast, serial-port and baud-rate.

This also means you should eventually edit the service file 2017/playbooks/files/etc/systemd/system/eer.service ExecStart method to not declare in these arguments.

The following code:

{% if inventory_hostname in groups['rasprime'] %}
ExecStart=/opt/topside-9.0.0/bin/{{ entry_point }} --broadcast 192.168.88.255 --serial-port /dev/ttyACM0 --baud-rate 115200
{% else %}
ExecStart=/opt/topside-9.0.0/bin/{{ entry_point }} --broadcast 192.168.88.255
{% endif %}

will become something like this:

ExecStart=/opt/topside-9.0.0/bin/{{ entry_point }} defaultConfig.yml overrideConfig.yml

Note {{ entry_point }} is an ansible-playbook variable which gets replaced with entry_point found in the playbook/hosts file. This happens when it is deployed to a system using a playbook template task:

  tasks:
    - name: Copy unit file for systemd
      template: src=files/etc/systemd/system/eer.service dest=/etc/systemd/system/eer.service mode=644

public final class Config {
private final ConfigurationProvider configProvider;

public Config() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Build with testing in mind; pass in the config file locations to this ctor.

See above comment about adding the config files as a command line option.

@ConnorWhalen
Copy link
Contributor Author

Addressed everything but the command line args. Going to do that next time and hopefully test it on rasprime thursday since i'll be touching the services

@cal-pratt
Copy link
Contributor

cal-pratt commented Mar 30, 2017

We probably need to change the location of the default config and the custom config. I think you could add it to playbooks/files/usr/bin playbooks/files/opt/topside.

I'd then add a task to the deploy playbook to copy these files to /opt/topside-9.0.0/ on each of the systems:

  tasks:
    - set_fact:
          version: "9.0.0"

...

- name: Add config files
  template: src=files/opt/topside/{{ item }} dest=/opt/topside-{{ version }}/{{ item }} mode=755 owner=pi group=pi
  with_items:
    - config.yml
    - defaultConfig.yml

Then update your service to reference the new location.


Edit: Why is our application folder on each machine called topside? How have I only just noticed this?

Copy link
Contributor

@cal-pratt cal-pratt left a comment

Choose a reason for hiding this comment

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

Looking good; added a few notes. You'll be able to deploy this to the system once you update the topside launcher ExecStart and you set up the LauncherConfig in the Topside and PiCamera main methods.

template: src=files/{{ item }} dest=/opt/eer-{{ version }}/{{ item }} mode=755 owner=pi group=pi
with_items:
- config.yml
- defaultConfig.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just add this directly to the deploy.yml script.
It'll also need to be hosts: rasprime:picamera

{% else %}
ExecStart=/opt/topside-9.0.0/bin/{{ entry_point }} --broadcast 192.168.88.255
{% endif %}
ExecStart=/opt/topside-9.0.0/bin/{{ entry_point }} --default /opt/topside-9.0.0/defaultConfig.yml --config /opt/topside-9.0.0/config.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also use {{ version }} here; the files are being copied to the hosts as a template, so the version variable will get replaced.

@ConnorWhalen ConnorWhalen force-pushed the configurable-rov branch 2 times, most recently from 1fd054c to 174e06b Compare April 6, 2017 22:54
Copy link
Contributor

@cal-pratt cal-pratt left a comment

Choose a reason for hiding this comment

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

I think you might have committed in some files by mistake; other than that some small points... keep it coming!

{% else %}
ExecStart=/opt/eer-9.0.0/bin/{{ entry_point }} --broadcast 192.168.88.255
{% endif %}
ExecStart=/opt/eer-9.0.0/bin/{{ entry_point }} --default /opt/eer-9.0.0/defaultConfig.yml --config /opt/eer-9.0.0/config.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

hit up that {{ version }} tag

playbooks/hosts Outdated

[rasprime]
192.168.88.4 ansible_user=pi ansible_ssh_pass=raspberry entry_point=rov
192.168.88.10 ansible_user=pi ansible_ssh_pass=raspberry entry_point=rov
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this change. We should update the hardware address in the dhcp configs. This ip is dhcp assigned; randomly selected from 10-99 (just so happens most routers assign in order). There is no guarantee that the raspi will always have this address.

@@ -1,4 +1,35 @@
- hosts: topside
- hosts: captain
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible mistake here? Surely we dont want to remove the proxy info 🔨


final File overrideConfigFile = new File(overrideConfigFilename);
final Environment overrideEnvironment = new ImmutableEnvironment(defaultConfigFile.getParent());
if (new File(overrideConfigFilename).exists()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You already have a file object! no need for a new one here

@ConnorWhalen ConnorWhalen force-pushed the configurable-rov branch 2 times, most recently from 40c85f5 to 19321a5 Compare April 23, 2017 15:45
@cal-pratt
Copy link
Contributor

Rebase these changes on top of origin master.. then ship it!

@cal-pratt cal-pratt force-pushed the configurable-rov branch from a1f2ea0 to 6114f57 Compare May 1, 2017 14:23
@cal-pratt cal-pratt merged commit 1ad5cb2 into master May 1, 2017
@cal-pratt cal-pratt deleted the configurable-rov branch May 1, 2017 14:27
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.

2 participants