Skip to content
This repository was archived by the owner on Jul 3, 2023. It is now read-only.

Add support for sending messages from NUsight#232

Merged
JosephusPaye merged 6 commits intomasterfrom
paye/send_messages_from_nusight
Feb 7, 2019
Merged

Add support for sending messages from NUsight#232
JosephusPaye merged 6 commits intomasterfrom
paye/send_messages_from_nusight

Conversation

@JosephusPaye
Copy link
Member

This adds support for sending messages from NUsight onto the network.

Added a new method send(message: Message, target?: string): void on src/client/network/network.ts, used by components in their local network file.

Informally tested using a simulator in #208.

Copy link
Member

@TrentHouliston TrentHouliston left a comment

Choose a reason for hiding this comment

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

The parser still needs a little work, and it would be good to make the API consistent for sending regardless of where it's sent from

/**
* Send the given message on the network
*/
send(message: Message, target?: string): void {
Copy link
Member

Choose a reason for hiding this comment

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

Why make a new Message type with a target as a 2nd parameter here over using NUClearNetSend?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to accept a NUClearNetSend.

import * as XXH from 'xxhashjs'

import { createSingletonFactory } from '../../shared/base/create_singleton_factory'
import { hashType } from '../../shared/nuclearnet/nuclearnet_proxy_parser'
Copy link
Member

Choose a reason for hiding this comment

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

I don't know where this function should go, both of these places seem like a bad spot.
How do we not have a utility location yet :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved back :)

Copy link
Member

Choose a reason for hiding this comment

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

How do we not have a utility location yet :P

NEVER!!! 🙂

case 'packet': {
const { id, data: [key, { target, type, payload, reliable }] } = packet
return callback([
JSON.stringify({ id, nsp, key, header: { target, type, reliable } }),
Copy link
Member

@TrentHouliston TrentHouliston Feb 7, 2019

Choose a reason for hiding this comment

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

You're sending type as a string here, and adding a hashType below that isn't a part of a NUClearNetSend packet.
The decoding logic below will take that and add a .hash element to it afterwards that'll be ignored, so the hashType(type) here is doing nothing.
However, NUClearNetSend does accept a buffer or a string here, so maybe instead don't send the type in json and in the parser below add it as the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the decoder to not add the unused hash.

@BrendanAnnable BrendanAnnable temporarily deployed to nusight-pr-232 February 7, 2019 05:48 Inactive
Copy link
Member

@TrentHouliston TrentHouliston left a comment

Choose a reason for hiding this comment

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

LGTM!

@BrendanAnnable BrendanAnnable requested a deployment to nusight-pr-232 February 7, 2019 06:23 Abandoned
@JosephusPaye JosephusPaye merged commit 5bd5498 into master Feb 7, 2019
@TrentHouliston TrentHouliston deleted the paye/send_messages_from_nusight branch February 7, 2019 06:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants