[PR] Align xxx_MODULE_ID_yyy usage.

[HansUweRempler]

Issue by amushatov Thursday Nov 14, 2019 at 11:15 GMT Originally opened as https://github.com/Bosch-AE-SW/cddk-oss/issues/382

---

In order RetCode and Logging subsystems to work properly, every module (i.e., each build unit) should define proper MODULE_ID macros. In addition, values assigned to each MODULE_ID must be unique for entire system. Currently we have modules which do not define this macros. Also values defined in BSP_CommonGateway_Modules_E, KISO_Essentials_ModuleID_E, KISO_CELLULAR_ModuleID_E, App_ModuleID_E and possibly others overlap.

[HansUweRempler]

Comment by ChiefGokhlayehBosch Thursday Nov 14, 2019 at 21:16 GMT

---

Please note, Module IDs were designed to be unique only within their package (i.e. Cellular, Essentials, Utils, etc.). To differentiate Retcodes with the same Module ID but coming from different packages we declare a KISO_PACKAGE_ID during compilation. Currently this seems to be hard-coded to 0 for every package (which is not intended). Either revisit the idea of what a package is, or properly set the package ID as intended by the Retcode system.

One side-note perhaps, in the past we only set KISO_PACKAGE_ID as compile time define. The value could never be retrieved during runtime as no symbol was attributed to it. Using CMakes configure_file function, we could define the value once in the build-system and then transfer it over to some const int CellularPackageId = ... during compilation. The specifics of this still need to be figured out though.

[HansUweRempler]

Comment by amushatov Friday Nov 15, 2019 at 08:14 GMT

---

What is making you to believe ModuleID should be unique per subsystem and not globally? What is making you to assume what is its intend in regards to Retcode system? However, there are limitations in other subsystems, which can convince me to change PackageID and leave ModuleID as is. PackageID and ModuleID are intended to work in tandem, and resulting tuple of those 2 must be unique. Any way, at present we don't use both of them correctly.

KISO_PACKAGE_ID can be retrieved at any time because it is available at compile time AND at runtime as constant. CMakes configure_file() is not needed for this, also separate (global) const variable is not needed. If you know the name of such (global) cont, you will know the name of the macros too.

[HansUweRempler]

Comment by ChiefGokhlayehBosch Friday Nov 15, 2019 at 11:34 GMT

---

What is making you to believe ModuleID should be unique per subsystem and not globally?

It's how we originally designed it, back when the project was was still called "CoSP". And this how it is used in XDK derivative of CoSP to this day. True, documentation is missing in-code, that's because we used to document package IDs and the internals of the Retcode system outside of the code-base (similar to how we do with Hugo right now). There's also a practical limitation: Module IDs only allocated one byte of the 4 byte Retcode bitfield, meaning one can only ever have 255 (2⁸-1 because 0 is reserved) unique modules. Note that I'm not arguing for or against a unique package ID, I just wanted to point out, that we used to view module IDs only unique within their package (as you said, the tuple must be unique).

This is what I found in the code-base: https://github.com/Bosch-AE-SW/cddk-oss/blob/ed65070f68fa8271ccb6f1f8ae6b8c52d60ddaf2/core/essentials/include/Kiso_Retcode.h#L229-L233

If you argue that package IDs are superseded due to how the repo is structured right now, ok! Let's bring this to the attention of all architects. If we scrap the idea of packages, we should also remove their allocation in the 32 bit Retcode bitfield and perhaps expand the module ID bitmask.

KISO_PACKAGE_ID can be retrieved at any time because it is available at compile time AND at runtime as constant.

I think you misunderstood me. Here's what I meant:

Say we implement a centralized error handler for any non-caught Retcode in our Kiso-enabled App (anything raised through Retcode_RaiseError() or Retcode_RaiseErrorFromIsr()). In this error handler we want to identify which package the Retcode came from. Simple enough right? You can retrieve the package ID from the Retcode via Retcode_getPackage(), wonderful! Now what do you compare it against... KISO_PACKAGE_ID? No, that would be the package ID of your current package. The one you are in right now. How do we obtain the numerical value of the package ID for Cellular? ... Do you see the problem? There is no KISO_PACKAGE_ID_CELLULAR defined anywhere! KISO_PACKAGE_ID is always populated with the value of your current package. You can not use it to obtain the package IDs of other packages.

[HansUweRempler]

Comment by amushatov Friday Nov 15, 2019 at 15:03 GMT

---

You and problems i faced in code (not only retcode bitmask) convinced me to keep ModuleID unique only under its containing package. That means i fill try to define PackageID somehow in order to get unique tuples. ModuleID is 'reserved' as fallback for non compliant modules. I will try to remove this too. NOTE: incorrectly structured files, which first include Retcode.h and after that define ModuleID are just bad formatted ones and does not belong to non compliant ones.

Out of the topic: Usage example you quoted, defining constraints on PackageID and ModuleID, is located at lines 230-233. Will move this example to top file header, thanks for pointing it out. Retcode_RaiseError() or Retcode_RaiseErrorFromIsr() does not enforce in any way that PackageID/ModuleID will be set. Retcode_RaiseError(RETCODE_FAILURE) is perfectly fine. From other side, some can freely return RETCODE(RETCODE_SEVERITY_NONE, RETCODE_SUCCESS) and it will be different from RETCODE_SUCCESS. Yet another example is return RETCODE_FAILURE;

[HansUweRempler]

Original pull request from transferring private to public repo -> closed