Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bridges update #2322

Merged
merged 13 commits into from
Apr 2, 2024
Merged

Conversation

Williangalvani
Copy link
Member

@Williangalvani Williangalvani commented Jan 19, 2024

This splits udp port handling into udp target port and udp lister port.

client will always send data to udp_target_port.
server will always listen at udp_listen_port.
additionally, this now allows the client to pick the port it will bind to, using udp_listen_port.
this is useful for implementations where whatever is talking to us is sending/receiving packets naively, without an actual udp "connection".

depends on patrickelectric/bridges#11

fix #2305
fix #2304

@Williangalvani Williangalvani force-pushed the bridges_update branch 2 times, most recently from cf77796 to 1bd4cb3 Compare January 19, 2024 17:01
@@ -15,6 +15,7 @@ MAV_COMPONENT_ID_ONBOARD_COMPUTER4=194
# Enable Rust backtrace for all programs
RUST_BACKTRACE=1

RUN_AS_BLUEOS_USER="sudo -u blueos"
Copy link
Member

Choose a reason for hiding this comment

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

in case sudo is not available, you can su blueos -c "commandline with arguments"

Copy link
Member Author

Choose a reason for hiding this comment

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

the quotes in there make commands insanely hard to assemble. Do you want to take a look at it? I tried for a while but ended up giving up

Copy link
Member

Choose a reason for hiding this comment

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

it should be something like:

BLUEOS_USER='blueos'

run_as_blueos_user () {
  su "$BLUEOS_USER" -c "$1"
}

run_as_blueos_user 'ls -lah /dev/video*'

Copy link
Member Author

Choose a reason for hiding this comment

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

didn't quite work, I think this need to be on its own file so it can be used inside the new tmux sessions.
I'll try it in a subsequent pr

@Williangalvani
Copy link
Member Author

"udp_listen_port": spec["udp_port"] if server else 0,
}
)
data["specsv2"] = new_specs
Copy link
Member

Choose a reason for hiding this comment

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

why not just replacing specs directly ?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good, that is just how my brain works. updated.

@@ -85,6 +86,12 @@ else
echo "Waring: Using dockers's resolv.conf, this may cause DNS issues if the host's network configuration changes."
fi

# fix permissions for logging folders:
find /var/logs/blueos -type d -exec chmod 777 {} \;
Copy link
Member

Choose a reason for hiding this comment

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

The default folder permission should be "755", no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, updated

self._bridges: Dict[BridgeSpec, Bridge] = {}
self._settings_manager = Manager("bridget", SettingsV1)
self._bridges: Dict[BridgeFrontendSpec, Bridge] = {}
self._settings_manager = Manager("bridget", SettingsV2, USERDATA / "settings" / "bridget")
Copy link
Member

Choose a reason for hiding this comment

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

Why are we moving this to userdata over our standard settings folder ?

Copy link
Member Author

Choose a reason for hiding this comment

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

non-root users have no access to our regular folder because it is under /root/

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a comment explaining that in the code

Copy link
Member

@patrickelectric patrickelectric left a comment

Choose a reason for hiding this comment

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

Be free to merge it

@Williangalvani
Copy link
Member Author

cool, I'm trying @joaoantoniocardoso 's suggestion and I'll merge it after I'm done

@Williangalvani Williangalvani force-pushed the bridges_update branch 2 times, most recently from 41f36e8 to dc4cb6b Compare April 2, 2024 16:38
@Williangalvani Williangalvani merged commit 49dd2e3 into bluerobotics:master Apr 2, 2024
6 checks passed
@Williangalvani Williangalvani deleted the bridges_update branch April 2, 2024 19:36
@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Apr 4, 2024
patrickelectric added a commit to patrickelectric/BlueOS-docker that referenced this pull request Jul 9, 2024
The previous code did not work since it was running with the extensions folder that can have a huge number of files
Not sure about the original work done in: bluerobotics#2322

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Williangalvani pushed a commit that referenced this pull request Jul 10, 2024
The previous code did not work since it was running with the extensions folder that can have a huge number of files
Not sure about the original work done in: #2322

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
4 participants