Skip to content

Support MROV UDP messages#15

Open
figuernd wants to merge 3 commits into
mainfrom
mrov
Open

Support MROV UDP messages#15
figuernd wants to merge 3 commits into
mainfrom
mrov

Conversation

@figuernd

Copy link
Copy Markdown
Contributor

Adding support for UDP streams coming from the MROV project. Enhanced logging for easier debugging.

@figuernd figuernd requested a review from rgov September 23, 2025 17:47
Comment on lines +284 to +285
# MROV packet format:
# MDS YYYY/MM/DD HH:MM:SS.SSS Lat Lon Z Depth Alt SOG COG Northing Easting Roll Pitch Heading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing I would like to have for each message type is a fully populated example, not only the field labels. This way we always have a documented test case at hand if we need it (though it would be smart to like, actually have test cases).

data = packet.decode().rstrip('\n').split(' ')

if not data or data[0] != 'MDS':
logger.debug('MDS line header missing')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sometimes we have multiplexed streams with different message types coming in on the same port, so I wouldn't log or it could get very noisy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's just a debug message, only logged if you set --log-level DEBUG

Comment on lines +312 to +324
# Extract MROV fields and map to standard nav data order
# MROV: MDS timestamp Lat Lon Z Depth Alt SOG COG Northing Easting Roll Pitch Heading
field_values = [
data[3], # latitude -> latitude
data[4], # longitude -> longitude
data[10], # northing -> local_x
data[11], # easting -> local_y
data[12], # roll -> roll
data[13], # pitch -> pitch
data[14], # heading -> heading
data[6], # depth -> depth
data[7], # altitude -> altitude
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure this is necessary (and it's a little confusing). Yes, the data_array will come out in a different order, but it's a key-value mapping in a trench coat.

If there's a display issue in Sealog, we should refactor the nav display to force the ordering. But I thought we might already do that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was mostly trying to keep the data identical to other products because I have no idea what kinds of downstream scripts might be out there using indices instead of keys.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm pretty confident there are none. If there were, I would be happy to break them so we can make them more robust (there is clearly one correct way to pull an entry out of a list of (key, value) pairs). I think there's a risk of ossification we need to resist or the system crosses a critical threshold of fragility.

Comment on lines +410 to +414
logger.debug(f'Found aux data for event {event_id}: timestamp={aux_data_ts}, data_source={aux_data.get("data_source", "unknown")}')
logger.debug(f'Aux data details: {aux_data}')
except ValueError as e:
logger.warning(f'No aux data available for event {event["message"]["id"]}: {e}')
logger.debug(f'Cache state - size: {len(AUX_DATA_CACHE)}, timestamps: {[entry.timestamp for entry in AUX_DATA_CACHE[:5]]}...')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some very long lines used for logging here and below, could those be wrapped please?

if abs((event_ts - aux_data_ts).total_seconds()) > ARGS.max_age:
logger.info('Ignoring event older than maximum age')
if time_diff > ARGS.max_age:
logger.info(f'Ignoring event {event_id} - aux data too old (age: {time_diff}s > max: {ARGS.max_age}s)')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably want {:.1f} or {:.0f} when printing time_diff due to precision.

Might be clearer: Not adding stale aux data to {event_id} (age: {time_diff:.1f}s > max: {ARGS.max_age}s)

else:
logger.warning(f'API request failed for event {event_id}: status={response.status_code}, response={response.text}')
except Exception as e:
logger.error(f'Exception occurred while posting aux data for event {event_id}: {e}')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use logger.exception() in except blocks. But actually here if we are Python 3.11 (and we ought to be by now), we can do

except Exception as e:
    e.add_note(f'Exception occurred while posting aux data for event {event_id}')
    raise

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately... 3.9

FROM python:3.9

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We control our own destiny

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants