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

Deprecate Router Client role (and make it Client) #4201

Merged
merged 1 commit into from
Jun 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/mesh/FloodingRouter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ bool FloodingRouter::shouldFilterReceived(const meshtastic_MeshPacket *p)
if (wasSeenRecently(p)) { // Note: this will also add a recent packet record
printPacket("Ignoring incoming msg we've already seen", p);
if (config.device.role != meshtastic_Config_DeviceConfig_Role_ROUTER &&
config.device.role != meshtastic_Config_DeviceConfig_Role_ROUTER_CLIENT &&
config.device.role != meshtastic_Config_DeviceConfig_Role_REPEATER) {
// cancel rebroadcast of this message *if* there was already one, unless we're a router/repeater!
Router::cancelSending(p->from, p->id);
Expand Down
1 change: 0 additions & 1 deletion src/mesh/RadioInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,6 @@ uint32_t RadioInterface::getTxDelayMsecWeighted(float snr)
uint8_t CWsize = map(snr, SNR_MIN, SNR_MAX, CWmin, CWmax);
// LOG_DEBUG("rx_snr of %f so setting CWsize to:%d\n", snr, CWsize);
if (config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER ||
config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER_CLIENT ||
config.device.role == meshtastic_Config_DeviceConfig_Role_REPEATER) {
delay = random(0, 2 * CWsize) * slotTimeMsec;
LOG_DEBUG("rx_snr found in packet. As a router, setting tx delay:%d\n", delay);
Expand Down
4 changes: 4 additions & 0 deletions src/modules/AdminModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,10 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c)
LOG_DEBUG("Tried to set node_info_broadcast_secs too low, setting to %d\n", min_node_info_broadcast_secs);
config.device.node_info_broadcast_secs = min_node_info_broadcast_secs;
}
// Router Client is deprecated; Set it to client
if (c.payload_variant.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER_CLIENT) {
config.device.role = meshtastic_Config_DeviceConfig_Role_CLIENT;
}
break;
case meshtastic_Config_position_tag:
LOG_INFO("Setting config: Position\n");
Expand Down
4 changes: 2 additions & 2 deletions src/modules/esp32/StoreForwardModule.cpp
Copy link
Member

@GUVWAF GUVWAF Jun 29, 2024

Choose a reason for hiding this comment

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

Shall we then also get rid of the requirement that a StoreForward server needs to be a ROUTER/ROUTER_CLIENT (line 536)? I think that might also be a reason why it was used a lot.

Problem is we currently don't have a StoreForward setting for "Client" or "Server", it's induced based on the role.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I now see this was discussed in #4185.
Store & Forward doesn't make sense for a mobile node, but I have a powered T-Beam inside I use as Store & Forward router and a RAK on the roof with much better coverage. I would rather have the T-Beam CLIENT_MUTE and still be able to get missed messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should we determine to put s&f in client mode? We used Router + Router client to determine this previously.
Instead of deprecating Router Client, should we perhaps rename to STATION to indicate a normal client which is at a fixed location and enable the serving mode of s&f for that role and router?

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to have it not be dependent on the role, but a dedicated S&F setting.

Copy link
Member

Choose a reason for hiding this comment

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

How should we determine to put s&f in client mode? We used Router + Router client to determine this previously. Instead of deprecating Router Client, should we perhaps rename to STATION to indicate a normal client which is at a fixed location and enable the serving mode of s&f for that role and router?

Let's not create another traffic issue while fixing this mistake, making store and forward easier to enable will likely negate gains from fixing router client misuse.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this is going to fix the issue. People who want Store and Forward are just going to have to run in Router mode now and manage with the admin channel.

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 believe this is going to fix the issue. People who want Store and Forward are just going to have to run in Router mode now and manage with the admin channel.

I see very little store and forward traffic compared to nodes set to router client.

Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,8 @@ ProcessMessage StoreForwardModule::handleReceived(const meshtastic_MeshPacket &m
#ifdef ARCH_ESP32
if (moduleConfig.store_forward.enabled) {

// The router node should not be sending messages as a client. Unless he is a ROUTER_CLIENT
if ((getFrom(&mp) != nodeDB->getNodeNum()) || (config.device.role == meshtastic_Config_DeviceConfig_Role_ROUTER_CLIENT)) {
// The router node should not be sending messages as a client
if ((getFrom(&mp) != nodeDB->getNodeNum())) {

if ((mp.decoded.portnum == meshtastic_PortNum_TEXT_MESSAGE_APP) && is_server) {
auto &p = mp.decoded;
Expand Down
Loading