sfc-aqua / quisp

Open source implementation of quantum internet simulation package
BSD 3-Clause "New" or "Revised" License
86 stars 36 forks source link

Add application id to connection setup messages #507

Closed makotonakai closed 1 year ago

makotonakai commented 1 year ago

Fix

435

What I have done so far

The problem that I am currently encountering

The unit test for ConnectionManager (RespondToRequest) does not work on my local environment after I added the setApplication_id / getApplication_id, because the respondToRequest method itself creates ConnectionSetupResponse with the given application id, while the unit test creates the ConnectionSetupRequest using the (classical?) message outside the method


This change is Reviewable

makotonakai commented 1 year ago

Reviewed 7 of 7 files at r1, all commit messages. Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @makotonakai)

_quisp/messages/connection_setup_messages.msg line 8 at r1 (raw file):_

packet ConnectionSetupRequest extends Header
{
    int application_id;

use @getter(getApplicationId) and @setter(setApplicationId) for accessors name. https://doc.omnetpp.org/omnetpp/manual/#sec:msg-defs:properties

_quisp/modules/Application/Application.h line 33 at r1 (raw file):_

 protected:
  int application_id;

how should we specify the application_id?

Reviewed 7 of 7 files at r1, all commit messages. Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @makotonakai)

_quisp/messages/connection_setup_messages.msg line 8 at r1 (raw file):_

packet ConnectionSetupRequest extends Header
{
    int application_id;

use @getter(getApplicationId) and @setter(setApplicationId) for accessors name. https://doc.omnetpp.org/omnetpp/manual/#sec:msg-defs:properties

_quisp/modules/Application/Application.h line 33 at r1 (raw file):_

Roger that

 protected:
  int application_id;

how should we specify the application_id?

That's what I'm working on. I'll add more commits if I figure that out.

makotonakai commented 1 year ago

Reviewed 5 of 5 files at r2, all commit messages. Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @makotonakai)

_quisp/messages/connection_setup_messages.msg line 8 at r2 (raw file):_

packet ConnectionSetupRequest extends Header
{
    int application_id @getter(getApplication_id) @setter(setApplication_id);

I don't want to mix the cases (snake_case and CamelCase). so getApplicationId and setApplicationId are better. we should use easy-to-expect names. I don't want to be confused getApplicationId or getApplication_id.

Sorry, I just fixed this

makotonakai commented 1 year ago

_quisp/modules/Application/Application.h line 33 at r1 (raw file):_

Previously, makotonakai (Makoto "dave" Nakai) wrote… btw, do you plan to send multiple requests which have different id from one Application module in your project?

Not for my project, but it would be necessary in the future, especially if some users want to simulate several applications in the same network. I guess this can be possible if I add another id (e.g. message_id) and increment it as the number of messages increases

makotonakai commented 1 year ago

_quisp/modules/Application/Application_test.cc line 44 at r2 (raw file):_

  virtual ~AppTestTarget() { EVCB.gateDeleted(toRouterGate); }
  std::unordered_map<int, int> getEndNodeWeightMap() { return this->end_node_weight_map; }
  int getId() { return this->id; }

do we need this? it's a public member, so we can access to it with app->id.

No, it's protected one, so that's why I added a function to retrieve that like the ones for other protected members. https://github.com/makotonakai/quisp/blob/add-application-id/quisp/modules/Application/Application.h#L32

makotonakai commented 1 year ago

Sorry for my late replies @zigen -san

makotonakai commented 1 year ago

@zigen -san Could you tell me where the documentation is located?

makotonakai commented 1 year ago

@zigen -san, I need your additional approval cuz I fixed some unit tests that failed in the previous commit...