search

inorbit-ai / vda5050_adapter_examples

BSD 3-Clause "New" or "Revised" License
13 stars 12 forks source link

Enable Isaac Mission Dispatch and VDA5050 Connector to work together #1

Closed nv-yuzho closed 2 years ago

leandropineda commented 2 years ago

Thanks for your PR @nv-yuzho ! Changes looks good to me.

I was wondering why you changed the network mode to host. Are you using a Mission Dispatch service locally that connects to the containers?

nv-yuzho commented 2 years ago

Thanks for your PR @nv-yuzho ! Changes looks good to me.

I was wondering why you changed the network mode to host. Are you using a Mission Dispatch service locally that connects to the containers?

Hi, @leandropineda! Yes, I'm testing everything locally for now. But if I didn't set the network to host, the order1 example in this repo didn't seem to work either.

leandropineda commented 2 years ago

Now I see why, thanks for the fix. LGTM!

nv-yuzho commented 2 years ago

Now I see why, thanks for the fix. LGTM!

No problem!

nv-yuzho commented 2 years ago

Could this PR be merged?

leandropineda commented 2 years ago

Yes, please go ahead and merge it

nv-yuzho commented 2 years ago

Yes, please go ahead and merge it

I think I don't have the write access to get this merged.

leandropineda commented 2 years ago

Indeed, I'll merge it then.

nv-yuzho commented 2 years ago

Thank you!

fmros commented 2 years ago

Due to this commit it is not possible to run the Isaac Mission Dispatch together with this package, since the network_mode is not set to local_host in the docker-compose file. What do you think to leave the settings merged in this PR as a separate branch?

leandropineda commented 2 years ago

Due to this commit it is not possible to run the Isaac Mission Dispatch together with this package, since the network_mode is not set to local_host in the docker-compose file. What do you think to leave the settings merged in this PR as a separate branch?

@fmros I generally try to avoid using host network mode unless strictly necessary. Also, we want to avoid having additional branches so we keep maintenance effort as low as possible.

We can make containers spawned by both docker compose files see each other by using the networks construct. For doing so we'd need to do two things:

  1. Modify the docker compose file on this repository to create a named bridge network

    ...
networks:
vda5050-adapter-examples:
driver: bridge

  2. Modify the Isaac Mission Dispatch docker compose file to use the same network:

    ...
networks:
front_vda5050-adapter-examples:
external: true

The Mission Dispatch docker compose also creates an MQTT container which might be confusing or error prone. Perhaps you could add a separate docker compose file that links to the vda5050-adapter-examples network without the MQTT instance. This way we can integrate both projects seamlessly while keeping the current Mission Dispatch compose file unchanged. WDYT?

nv-yuzho commented 2 years ago

Due to this commit it is not possible to run the Isaac Mission Dispatch together with this package, since the network_mode is not set to local_host in the docker-compose file. What do you think to leave the settings merged in this PR as a separate branch?

@fmros I generally try to avoid using host network mode unless strictly necessary. Also, we want to avoid having additional branches so we keep maintenance effort as low as possible.

We can make containers spawned by both docker compose files see each other by using the networks construct. For doing so we'd need to do two things:

  1. Modify the docker compose file on this repository to create a named bridge network
...
networks:
  vda5050-adapter-examples:
    driver: bridge
  1. Modify the Isaac Mission Dispatch docker compose file to use the same network:
...
networks:
  front_vda5050-adapter-examples:
    external: true

The Mission Dispatch docker compose also creates an MQTT container which might be confusing or error prone. Perhaps you could add a separate docker compose file that links to the vda5050-adapter-examples network without the MQTT instance. This way we can integrate both projects seamlessly while keeping the current Mission Dispatch compose file unchanged. WDYT?

Yeah, I think the networks construct is a better solution! We will make the change on the Mission Dispatch side and test it out with the new changes you will make. Thank you. @leandropineda