Open
Conversation
…ring popoto_driver startup in Ethernet mode
…sn't point to the correct acked frame.
…h multiple arguments due to a problem in popoto API. Making a separate signal_and_write_raw() function for TransmitJSON commands with multiple arguments
Member
|
Thanks @SupunRandeni - this is great. @Thomas-Mccabe and @JaredSilbermann: Could you take a look at this and let me know if you have any concerns or input before I merge it? I would like to consolidate around a single working Popoto driver but don't currently have the hardware or need to test this myself. |
There was a problem hiding this comment.
Pull request overview
Fixes and extends the Popoto modem driver, primarily for TCP client deployments, and adds initial two-way ranging support.
Changes:
- Updates TCP client startup to use
DriverConfig.tcp_server/tcp_portand avoids serial-only commands in TCP mode. - Reworks Popoto TX command emission (raw JSON for TCP) and adjusts Goby/Popoto header + ACK handling.
- Adds Popoto two-way ranging request/response plumbing and extends ranging reply fields.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
src/acomms/protobuf/popoto_driver.proto |
Updates Popoto driver-specific transmission types and ranging reply fields. |
src/acomms/modemdriver/popoto_driver.h |
Adds raw-write helper and internal state for ranging message parsing. |
src/acomms/modemdriver/popoto_driver.cpp |
Implements TCP raw JSON commands, updates startup config usage, and adds ranging + header parsing changes. |
src/acomms/modemdriver/driver_base.cpp |
Prevents null deref by checking modem_ before calling active(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+417
to
+419
| // A range messages needs to be formated with 'range txPower' we will use the default power from .moos launch file | ||
| std::stringstream range; | ||
| range << "setvaluef range " << popoto_driver_cfg().modem_power() << "\n"; |
Comment on lines
+554
to
+556
| if (popoto0->cmd->isConnected()) { | ||
| popoto0->cmd->send_data((char*)raw.c_str(), (int)raw.length()); | ||
| } |
| } | ||
| message RangingReply | ||
| { | ||
| required double one_way_travel_time = 1 |
Comment on lines
+709
to
+714
| DecodeGobyHeader(data[0],data[1], modem_msg); | ||
| if (modem_msg.type() == protobuf::ModemTransmission::DATA) | ||
| *modem_msg.add_frame() = data.substr(1); // Header is 1 byte, correct? So this should be substr(1) not substr(2). -Supun- | ||
| else if (modem_msg.type() == protobuf::ModemTransmission::ACK){ | ||
| modem_msg.add_acked_frame(data[1]); | ||
| } |
Comment on lines
792
to
+804
| header |= ( m.frame_start() & 0b00111111 ); | ||
|
|
||
| // See if ack is requested, and encode it to the header. | ||
| // This part was missing; thus, DecodeGobyHeader() was misbehaving.. -Supun- | ||
| header &= ~(1 << GOBY_HEADER_ACK_REQUEST); // Clear bit 1 to set header to no-ack-request | ||
| if (m.ack_requested()){ | ||
| header |= (1 << GOBY_HEADER_ACK_REQUEST); // Set bit 1 is ack is requested | ||
| } | ||
|
|
||
| } else if (m.type() == protobuf::ModemTransmission::ACK){ | ||
| header |= ( GOBY_ACK_TYPE & 0b11 ) << 6; | ||
| header |= ( m.frame_start() & 0b00111111 ); | ||
| header &= ~(1 << GOBY_HEADER_ACK_REQUEST); // Clear bit 1 to set header to no-ack-request |
Comment on lines
+850
to
863
| uint8_t goby_header_type = (header >> 6) & 0b11; | ||
| switch (goby_header_type) | ||
| { | ||
| case GOBY_DATA_TYPE: | ||
| m.set_type(protobuf::ModemTransmission::DATA); | ||
| m.set_ack_requested( header & (1 << GOBY_HEADER_ACK_REQUEST)); | ||
| m.set_frame_start( ack_num ); | ||
| break; | ||
| case GOBY_ACK_TYPE: | ||
| m.set_type(protobuf::ModemTransmission::ACK); | ||
| m.set_frame_start(ack_num); | ||
| m.add_acked_frame( ack_num ); | ||
| break; | ||
| } |
Comment on lines
612
to
+620
| enum PopotoMessageType | ||
| { | ||
| DATA_MESSAGE = 0, | ||
| RANGE_RESPONSE = 128, | ||
| RANGE_RESPONSE = 138, | ||
| RANGE_REQUEST = 129, | ||
| STATUS = 130 | ||
| }; | ||
| // Popoto documentation (https://www.popotomodem.com/static/17704245ba22b453f7e532e2d40273ee/PMM5544UsersGuide.pdf page 279) says range response MessageID is 128. | ||
| // But looking at the actual range response messages, it is 138! -Supun- |
Comment on lines
+64
to
+68
| POPOTO_TWO_WAY_RANGE_REQUEST = 3; // Ranging request | ||
| POPOTO_TWO_WAY_RANGE_RESPONSE = 4; // Response that you receive to a ranging request | ||
| POPOTO_DEEP_SLEEP = 5; | ||
| POPOTO_WAKE = 6; | ||
| POPOTO_SET_TX = 7; |
| POPOTO_TWO_WAY_RANGE_REQUEST = 3, // used | ||
| POPOTO_TWO_WAY_RANGE_RESPONSE = 4 // used | ||
| }; | ||
| TransmissionTypeInternal transmissions_type_internal; |
Comment on lines
92
to
106
| else if (driver_cfg_.connection_type() == goby::acomms::protobuf::DriverConfig::CONNECTION_TCP_AS_CLIENT) | ||
| { | ||
| if (popoto_driver_cfg().local().has_ip() && popoto_driver_cfg().local().has_port()) | ||
| if (cfg.has_tcp_server() && cfg.has_tcp_port()) | ||
| { | ||
| std::string ip = popoto_driver_cfg().local().ip(); | ||
| std::string port = popoto_driver_cfg().local().port(); | ||
|
|
||
| std::string ip = cfg.tcp_server(); | ||
| // std::string port = popoto_driver_cfg().local().port(); | ||
|
|
||
|
|
||
| // Need to issue the disconnect command to stop pshell | ||
| signal_and_write("disconnect\n"); | ||
| // signal_and_write("disconnect\n"); | ||
|
|
||
| myConnection = ETHERNET_CONNECTION; | ||
| // Initialize with the IP, port and PCM callback routine. | ||
| popoto0 = new popoto_client(ip, stoi(port), Popoto0PCMHandler); | ||
| popoto0 = new popoto_client(ip, cfg.tcp_port(), Popoto0PCMHandler); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am using popoto modems for a project, and it was great to see that Mission Systems has already written a goby modem driver for it. I tried it out but I ran into some problems. I made some changes to fix them. These fixes have been tested in the field (using the TCP connection mode only). Putting up this PR to discuss and merge. Here is the summary of my changes:
In TCP mode, driver was trying to send serial commands to switch the modem to TCP. This assumes that we have both TCP and serial connections to the modem, which is unlikely. Also, the driver sends serial commands before initializing serial connection, resulting in a segfault. supun-randeni@e7b49e4 and supun-randeni@0642bf0 commits fix that.
When connection type is DriverConfig::CONNECTION_TCP_AS_CLIENT, cfg.tcp_server() and cfg.tcp_port() configs are required. Driver was using a custom popoto_driver_cfg().local().has_ip() instead, resulting in a configuration error. I modified to use the global cfg.tcp_server() and cfg.tcp_port() instead: supun-randeni@985e5ec
Popoto doesn't seem to support ack. SoMissionSystems have implemented a custom header to handle ack-required/ack-not-required, which is pretty cool. But there are some issues in that implementation.
If the ack is requested, they are sending an empty data packet to indicate ack. But this does not work. I think goby needs to know which frame is the ack associated with (acked_frame)? Right now, they just set 'frame_start' of the ack packet to 'acked_frame'. I haven't fixed it, but pointed out some comments here: supun-randeni@9760676
Popoto API has a small issue here on how they handle JSON commands. Because of that, we cannot use their SendCommand() function to send commands with multiple arguments. So I switched to sending raw JSON commands using one of their lower level functions. I'm pretty sure this issue will persist in the serial mode too. But I have only tested in TCP mode. So I made sure my changes apply only to the TCP mode, and serial mode stays as is: supun-randeni@1c77bb8
Implemented the two-way ranging functionality here: supun-randeni@f920d5f