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

Enhancement Request for Foreign Device Registration in IpNetwork #105

Open
Thisora opened this issue Nov 27, 2024 · 5 comments
Open

Enhancement Request for Foreign Device Registration in IpNetwork #105

Thisora opened this issue Nov 27, 2024 · 5 comments

Comments

@Thisora
Copy link

Thisora commented Nov 27, 2024

Hello,

First of all, thank you for your work on this library—it's been a great help!

I'm currently implementing a LocalDevice that needs to register itself as a foreign device on a remote BBMD (via FDT). Here’s a snippet of my code to provide context:

IpNetwork network = (IpNetwork) localDevice.getNetwork();
InetSocketAddress inetSocketAddress = new InetSocketAddress(InetAddress.getByName(server.getHostname()), server.getPort());
network.registerAsForeignDevice(inetSocketAddress, (int) timeToLive.getDuration().getSeconds());

I want to handle the scenario where the device is already registered in the same way as when registration succeeds.

Specifically:

  • If the registration succeeds: great!
  • If the registration fails because the device is already registered, I want the code to proceed as if the registration succeeded.
  • if registration fails due to other reasons (e.g., timeout), I want to handle that separately.

I'm trying to manage the case where my device is already registered:
I want that my code act as same if the device is register as if registration worked:

  • If registration success, well.
  • If registration failed but the device is already registered, well tho.

The problem is that registerAsForeignDevice throws a BACnetException in all failure cases, with the only distinguishing factor being the error message. This makes it hard to differentiate between these scenarios programmatically.

But if registration fail, (for already registered either other exception like timeout ) it return a BACnetException with the only difference is the error message.

My Current Workaround

As a workaround, I used reflection to access the normally private foreignBBMD field in IpNetwork to check if the device was already registered. Here’s the workaround code:

    private boolean wasAlreadyRegistered(final IpNetwork ipNetwork) {

        try {
            Field field = IpNetwork.class.getDeclaredField("foreignBBMD");
            field.setAccessible(true);
            InetSocketAddress foreignBBMD = (InetSocketAddress) field.get(ipNetwork);
            return foreignBBMD != null;

        } catch (Throwable t) {
            ...
        }
    }

This is, of course, not ideal because if field is renamed, code will break and there is no synchronization.

Proposed Improvements

It would be helpful if IpNetwork provided a public method to check whether the device is already registered as a foreign device. For example:

public boolean isDeviceRegisteredAsForeignDevice(){
    synchronized (foreignBBMDLock) {
            return foreignBBMD != null;
   }
}

In case there are some technical issue with exposing the method, I suggest to add more exceptions types. It would be much easier to handle errors programmatically if registerAsForeignDevice threw a more specific exception (or subclasses of BACnetException) to differentiate between cases like "already registered" and other failures (e.g., timeouts).

Thank you for considering this, and I’d be happy to clarify further if needed! I also can submit a PR.

@kishorevenki
Copy link

Hi Thisora,

As per the BACnet Standards, sending RegistrationAsForeignDevice to the same BBMD is acceptable even before the expiry of TimeToLive timer. In that case, the existing TimeToLive will be reset and new timer would start. Hence I feel it would be better to carry out such checking in the Application Layer.

Handling Registration Timeout error and NAK response is already handled separately.

@Thisora
Copy link
Author

Thisora commented Nov 28, 2024

Thank you for the clarification.

I wasn’t specifically looking to differentiate between handling the Registration Timeout error and the NAK response. My main concern is that I don’t want my application to behave the same way when the BACnetException comes from the library (e.g., already registered as a foreign device) as it does when the exception is related to the FDT registration itself (such as a timeout).

@kishorevenki
Copy link

Registering once again to the same BBMD while the registration is already active to the same BBMD is expected behavior and hence the library will not throw error. If the library throws error, then it is not conforming to the BACnet standard.

@Thisora
Copy link
Author

Thisora commented Nov 28, 2024

I mean, you mentioned that there is already a way to differentiate between a NAK and a Timeout, but what exactly is that way?

From what I see, the exception that gets thrown is always a BACnetException:

//from IpNetwork
if (bbmdResponse == -1)  
    throw new BACnetException("Foreign registration timeout");  
if (bbmdResponse != 0)  
    throw new BACnetException("NAK response: " + bbmdResponse);  

The only difference between these cases is the exception message string. Do I really need to parse the exception message to differentiate them?

Wouldn’t it make more sense to create distinct exception types for these cases so that we can handle them more cleanly? For example:

try {  
    // this code throws exceptions  
} catch (FdtRegistrationException e) {  
    // my app can act, maybe retry later  
} catch (BACnetException e) {  
    // my app can act differently  
}

And this is example show how users could use it in an ExceptionListener:

    @Override
    public void receivedException(final Exception e) {
        if(e instanceof BACnetException) {
            /// my app can act a way
        }else if (e instanceof FtdRegistrationException) {
            
        }
    }

This approach would make it easier to handle different scenarios without relying on parsing error messages.

@Thisora
Copy link
Author

Thisora commented Nov 28, 2024

If the library throws error, then it is not conforming to the BACnet standard.

Yes i agree, and it is what it does for now.

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

No branches or pull requests

2 participants