osmo-tetra/4a8bd75e1e5f05a62291f7271d7e4c223d122e06

{
  "comments": [
    {
      "unresolved": true,
      "key": {
        "uuid": "33b82f9f_99caaf4d",
        "filename": "src/tetra_upper_mac.c",
        "patchSetId": 3
      },
      "lineNbr": 44,
      "author": {
        "id": 1000004
      },
      "writtenOn": "2022-09-20T12:17:27Z",
      "side": 1,
      "message": "this introduces a global, static variable.   Normally all state should be contained witin the tetra_mac_state or whatever other related \u0027context\u0027 structure to ensure the code works also if there are many instances in parallel, without any clashes over global variables storing state that logically belongs to an instance.",
      "range": {
        "startLine": 44,
        "startChar": 0,
        "endLine": 44,
        "endChar": 1
      },
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "6894bd91_c3ed915d",
        "filename": "src/tetra_upper_mac.c",
        "patchSetId": 3
      },
      "lineNbr": 44,
      "author": {
        "id": 1000225
      },
      "writtenOn": "2022-09-20T14:01:14Z",
      "side": 1,
      "message": "The lower mac allocates a global context structure tetra_cell_data with global pointer tcd. I\u0027d figure that it doesn\u0027t belong in that context. We could, however, add an upper mac context (upper_mac_context umc?), which, for now, would only hold the fragslots. Would that be preferable?\n\nNote that passing the context by parameter will not work without changing a lot of unrelated prototypes, as that would imply the lower mac (and burst detector, etcetera) also need to have it. It would not be a bad idea to create a context in tetra-rx that contains the respective states for the burst detector, lower mac, upper mac and possibly layers above, but that would be a significant rework.",
      "parentUuid": "33b82f9f_99caaf4d",
      "range": {
        "startLine": 44,
        "startChar": 0,
        "endLine": 44,
        "endChar": 1
      },
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "7e02e433_691dc283",
        "filename": "src/tetra_upper_mac.c",
        "patchSetId": 3
      },
      "lineNbr": 44,
      "author": {
        "id": 1000004
      },
      "writtenOn": "2022-09-22T06:24:19Z",
      "side": 1,
      "message": "OK, then lets keep the global/static variables for now, but add a TODO comment that this actualY should be in some context",
      "parentUuid": "6894bd91_c3ed915d",
      "range": {
        "startLine": 44,
        "startChar": 0,
        "endLine": 44,
        "endChar": 1
      },
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "a2b9dee1_9d5a3db1",
        "filename": "src/tetra_upper_mac.c",
        "patchSetId": 3
      },
      "lineNbr": 50,
      "author": {
        "id": 1000004
      },
      "writtenOn": "2022-09-20T12:17:27Z",
      "side": 1,
      "message": "is there a specific reason to pre-allocate lots of message buffers here?",
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "32214bbc_8fb3be83",
        "filename": "src/tetra_upper_mac.c",
        "patchSetId": 3
      },
      "lineNbr": 50,
      "author": {
        "id": 1000225
      },
      "writtenOn": "2022-09-20T14:01:14Z",
      "side": 1,
      "message": "You\u0027re right, I could allocate them on demand, whenever a fragmented resource is encountered on a slot. I\u0027ll do that in the next patch set",
      "parentUuid": "a2b9dee1_9d5a3db1",
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "68417ba3_a33b8cf0",
        "filename": "src/tetra_upper_mac.c",
        "patchSetId": 3
      },
      "lineNbr": 50,
      "author": {
        "id": 1000004
      },
      "writtenOn": "2022-09-22T06:24:19Z",
      "side": 1,
      "message": "\u003e You\u0027re right, I could allocate them on demand, whenever a fragmented resource is encountered on a slot. I\u0027ll do that in the next patch set\n\nI\u0027m not saying it must be done that way, was just wondering if there was some specific reason.",
      "parentUuid": "32214bbc_8fb3be83",
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "48383659_ca5a1eda",
        "filename": "src/tetra_upper_mac.h",
        "patchSetId": 3
      },
      "lineNbr": 13,
      "author": {
        "id": 1000004
      },
      "writtenOn": "2022-09-20T12:17:27Z",
      "side": 1,
      "message": "sounds like \u0027active\u0027 could be of type bool?\nwould be nice to have some comments explaining the contents for the non-obvious ones such as \u0027age\u0027 and \u0027encrypytion\u0027.\n\nThe age comment should not just explain the units, but also explain the rationale of having a signed type for the age.\n\nFor \u0027encryption\u0027 it also would be good to explain what it is for.  Is it just a flag? or an indication of a specific algorithm?...  In the code I also see stuff like \u0027encryption \u003d encryption_mode\u0027 where the latter is an unsigned type.  So once again the question arises why a signed type is used to represent something that\u0027s unsigned.",
      "range": {
        "startLine": 13,
        "startChar": 0,
        "endLine": 13,
        "endChar": 3
      },
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "fa659b74_5992edd1",
        "filename": "src/tetra_upper_mac.h",
        "patchSetId": 3
      },
      "lineNbr": 13,
      "author": {
        "id": 1000225
      },
      "writtenOn": "2022-09-20T14:01:14Z",
      "side": 1,
      "message": "You\u0027re right, it\u0027s kind of underdocumented. I\u0027ll make sure to annotate the fields. Booleans are used nowhere in the osmo-tetra project, so I sticked with what seems to be convention. tetra_mac_state for instance uses a signed int for the is_traffic field. If you want, I can start using the bool type or make the active field and similar fields a uint8_t.",
      "parentUuid": "48383659_ca5a1eda",
      "range": {
        "startLine": 13,
        "startChar": 0,
        "endLine": 13,
        "endChar": 3
      },
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    },
    {
      "unresolved": true,
      "key": {
        "uuid": "fd4e73ca_93018ea3",
        "filename": "src/tetra_upper_mac.h",
        "patchSetId": 3
      },
      "lineNbr": 13,
      "author": {
        "id": 1000004
      },
      "writtenOn": "2022-09-22T06:24:19Z",
      "side": 1,
      "message": "this probably shows how our practices have evolved during the last decade. using bool in C might not have been advisable with comlers used 10+ years ago, but today it clearly is the right thing. please modify, thanks!",
      "parentUuid": "fa659b74_5992edd1",
      "range": {
        "startLine": 13,
        "startChar": 0,
        "endLine": 13,
        "endChar": 3
      },
      "revId": "4a8bd75e1e5f05a62291f7271d7e4c223d122e06",
      "serverId": "035e6965-6537-41bd-912c-053f3cf69326"
    }
  ]
}