Pull request #176 broke the FirmwareManagementProfile!

[robert-s-ubi]

Pull request #176 looked suspicious to me, and sure enough, it breaks things. Our client (charger) side implementation cannot send DiagnostisStatusNotificationRequests nor FirmwareStatusNotificationRequests anymore:

2022-03-18 14:36:34.181 I SAL OCPP_1_6_MSG_TX:"DiagnosticsStatusNotificationRequest"
2022-03-18 14:36:34.187 D DefaultSsoOcppCommunicationTask [571f8699]Uncritical Exception:
2022-03-18 14:36:34.187 D DefaultSsoOcppCommunicationTask [571f8699]eu.chargetime.ocpp.UnsupportedFeatureException
2022-03-18 14:36:34.187 D DefaultSsoOcppCommunicationTask [571f8699] at eu.chargetime.ocpp.Client.send(Client.java:150)
2022-03-18 14:36:34.187 D DefaultSsoOcppCommunicationTask [571f8699] at eu.chargetime.ocpp.JSONClient.send(JSONClient.java:163)

2022-03-18 14:37:39.846 I SAL OCPP_1_6_MSG_TX:"FirmwareStatusNotificationRequest"
2022-03-18 14:37:39.848 D DefaultSsoOcppCommunicationTask [93680eb1]Uncritical Exception:
2022-03-18 14:37:39.853 D DefaultSsoOcppCommunicationTask [93680eb1]eu.chargetime.ocpp.UnsupportedFeatureException
2022-03-18 14:37:39.853 D DefaultSsoOcppCommunicationTask [93680eb1] at eu.chargetime.ocpp.Client.send(Client.java:150)
2022-03-18 14:37:39.854 D DefaultSsoOcppCommunicationTask [93680eb1] at eu.chargetime.ocpp.JSONClient.send(JSONClient.java:163)

I would strongly recommend reverting this commit.

[robert-s-ubi]

Reverting this commit made the library functional again for us:
fb53e2d

[robert-s-ubi]

The more I look into this commit, the more bogus it appears: ALL ClientProfile and ServerProfile classes have this "Feature cross-over", so why should be changed in only one and not the others? Also, the "fix" was obviously done wrong: The features that were removed from the client class should have been removed from the server class and vice versa.

It seems the submitter either used the classes in the wrong way (Client for Central System and Server for Charger?), or never tested the changes at all. And the builtin tests which revealed how wrong the change was were ignored...

At the risk of sounding like a broken record: Pull request #176 should be reverted ASAP.

[TVolden]

Hi @robert-s-ubi ,

It's good to hear from you again.

I'm sorry to hear that the pull request has had unforseen consequences. I doubled checked the code with the specification before approving it, I'm sure I did. The failing tests wasn't ignored, they were removed after investigating the specs.

That being said, I do not doubt you, I must have missed (or confused) something. I'm under alot of pressure lately, sorry.

As far as I remember, that PR was mostly an enhancement/refactoring, so unless @in-fke has something to add, I'd suggest reverting it today.

Sincerely,
Thomas Volden

[in-fke]

Sorry to hear that things were broken. As to what I tested, it fixed things for me. Please feal free to revert it and we can look at it when we have more time. As to propagating problems like this. @robert-s-ubi to you integrate via Source Code? I prefer integrating released binaries. Though as for this project, we forked it in order to have a distinct version.

[TVolden]

Done. Please verify if it works.

[robert-s-ubi]

Hi Thomas,

thanks for the quick response. Looking into Client.java:

          @Override
          public Confirmation handleRequest(Request request) throws UnsupportedFeatureException {
            Optional<Feature> featureOptional = featureRepository.findFeature(request);
            if (featureOptional.isPresent()) {
              return featureOptional.get().handleRequest(getSessionId(), request);
            } else {
              throw new UnsupportedFeatureException();
            }
          }

  public CompletableFuture<Confirmation> send(Request request)
      throws UnsupportedFeatureException, OccurenceConstraintException {
    Optional<Feature> featureOptional = featureRepository.findFeature(request);
    if (!featureOptional.isPresent()) {
      logger.error("Can't send request: unsupported feature. Payload: {}", request);
      throw new UnsupportedFeatureException();
    }

It checks the feature list for both requests to be sent as well as requests it receives. The same is found in Server.java.

I.e. both Client and Server Profile class must always contain ALL the requests they send AND receive. So removing either will break things.

It might be "cleaner" to have separate feature lists for requests that are sent and requests that are received for each profile. Then there would be no ambiguity between client and server, as each request would only EITHER be sent OR received by one of these.

[robert-s-ubi]

Ah, taking a second look you actually had my "clean" approach implemented, and reverting restored it:

  public ServerFirmwareManagementProfile(ServerFirmwareManagementEventHandler eventHandler) {
    features = new HashSet<>();
    features.add(new GetDiagnosticsFeature(null));
    features.add(new DiagnosticsStatusNotificationFeature(this));
    features.add(new FirmwareStatusNotificationFeature(this));
    features.add(new UpdateFirmwareFeature(null));
  }

The "this" means it is a feature that is "owned" (received/handled) by this profile, whereas "null" means it is a feature that is sent by this profile. So this is all correct, the server (central system) sends GetDiagnostics and UpdateFirmware requests (marked "null"), and receives DiagnosticsStatusNotification and FirmwareStatusNotification requests (marked "this").

And the client side contains the counterpart assignments of "this" and "null":

  public ClientFirmwareManagementProfile(ClientFirmwareManagementEventHandler eventHandler) {
    features = new HashSet<>();
    features.add(new DiagnosticsStatusNotificationFeature(null));
    features.add(new FirmwareStatusNotificationFeature(null));
    features.add(new GetDiagnosticsFeature(this));
    features.add(new UpdateFirmwareFeature(this));
  }

So it was all perfectly well marked before commit #176, and after this revert, it is once again. Thank you!