Update lwipopts.h

[Neuromancer2701]

Enable broadcast by disabling Broadcast filters
resolve #3163

Description

enable broadcast functionality

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Steps to test or reproduce

Tested with python scripts on a NUCLEO_F767ZI

[0xc0170]

cc @geky @kjbracey-arm

[kjbracey]

Seems reasonable to me. Certainly the _RECV flag is non-standard socket behaviour (requiring a socket option to receive broadcasts). I'd say that shouldn't be ever on, it's just weird.

The other one - requiring a socket option to send broadcasts - is standard for IPv4 (because broadcast and non-broadcast addresses aren't trivially distinguishable), but we don't have that socket option exposed through the NSAPI so it could be turned on. So we have to rely on NSAPI implementations always sending broadcasts. Turning off the LWIP option does solve that.

Conceivably you might also want a defensive measure in the NSAPI implementation to make sure the broadcast flag is set on all sockets in case anyone does reconfigure LWIP to have that flag enabled.

[geky]

So if I understand correctly, does disabling IP_SOF_BROADCAST deviate from posix socket behaviour? Does #3491 provide a more standard solution?

+1 for disabling IP_SOF_BROADCAST_RECV

[0xc0170]

@Neuromancer2701

So if I understand correctly, does disabling IP_SOF_BROADCAST deviate from posix socket behaviour? Does #3491 provide a more standard solution?

Have you had a chance to look at this question and referenced PR?

[Neuromancer2701]

How do the filters interact with the broadcast option? So some combination of the two might be needed. Or Does the SO_BROADCAST socket option supersede the filters?

[sg-]

@kjbracey-arm @geky what are your feelings about this?

[adbridge]

bump @kjbracey-arm @geky

[kjbracey]

My feeling is that neither this pull nor #3491 is quite ideal, but that this is closer, and could go in. (The two changes are mutually exclusive).

The issue is that NSAPI using LWIP won't send or receive broadcasts. NSAPI using Nanostack the issue doesn't apply (IPv6 only, no broadcasts). NSAPI with ESP8266, I don't know what the state is.

The reason is that LWIP is currently built to require the SOF_BROADCAST option to be enabled to both send and receive broadcasts.

I think turning off IP_SOF_BROADCAST_RECV is definitely correct - this seems like a "do something broken" build option that changes the semantics of SOF_BROADCAST to be non-standard.

The remaining question is do we want to expose SOF_BROADCAST through NSAPI, like #3491 does, or just make broadcast transmission allowed in NSAPI by default?

My slight preference is for broadcast to be permitted by default. Keep it simple. Having an option that not all stacks necessarily pay attention to seems potentially error-prone for apps. So I prefer this patch rather than #3491.

My only slight concern that someone else might turn on IP_SOF_BROADCAST if they messed with config, so you might want to add this or equivalent in socket creation:

#if IP_SOF_BROADCAST
   s->conn->pcb.udp->so_options |= SOF_BROADCAST;
#endif
#if IP_SOF_BROADCAST_RECV
#error "Don't turn on IP_SOF_BROADCAST_RECV, it's weird"
#endif

Maybe we don't expect anyone to be messing that config, so you maybe it's this somewhere in NSAPI:

#if IP_SOF_BROADCAST || IP_SOF_BROADCAST_RECV
#error "NSAPI wants broadcasts by default - do not enable IP_SOF_BROADCAST in lwIP options"
#endif

I guess my point is that I don't want NSAPI behaviour to subtly change based on lwIP build options.

I'd like to hear @geky's thoughts. Anything from @mikaleppanen?

[sg-]

I'd like to hear @geky's thoughts. Anything from @mikaleppanen?

bump

[mikaleppanen]

I prefer this pull request over #3491. Also I think that broadcasts should be permitted by default to make it simpler for clients.

[adbridge]

@geky BUMP

[geky]

Sorry about the delay, I'm down for defaulting to broadcast always on.

Less work for users, less work for us, ¿slightly more alignment with ipv6/ipv4?, and it seems like everyone is on board with this approach.

To move forward on this, lets take this guy in (after CI). I'll close #3491 to avoid any confusion.
/morph test-nightly

[mbed-bot]

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1603

Test failed!

[bridadan]

I found a bug in one of the CI flows that was causing this PR to fail, so sorry for the issue! I'll restart it here now.

/morph test-nightly

[mbed-bot]

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1609

All builds and test passed!