[FEATURE] Add SoC time stamp forwarding feature in BSD semaphore design #229

powerlink-team-kalycito · 2017-04-17T04:21:00Z

Create a shared memory between kernel and user layer to store SoC
timestamps using mmap.
The initialized shared memory is unmapped during the exit of the
function.

This pull request also takes care of the comments from the previous pull request #227

- Create a shared memory between kernel and user layer to store SoC timestamps using mmap. - The initialized shared memory is unmapped during the exit of the function. Change-Id: Ia1e0926db05520ea7bf7e12c4a801d05e54c4fa9

zelenkaj · 2017-04-18T06:22:00Z

stack/src/kernel/timesync/timesynckcal-bsdsem.c

+        // Close the semaphore
+        sem_close(instance_l.syncSem);
+        // Unlink the semaphore
+        sem_unlink(TIMESYNC_SYNC_BSDSEM);


Please do the semaphore cleanup outside of this function!

Not handled semaphore clean up outside the function, instead the handling is done using goto statements inside the same function itself.

Code duplication: If you do sem_close() and sem_unlink() in timesynckcal_init() if createTimestampShm() fails, you can workaround code duplication!

zelenkaj · 2017-04-18T06:22:07Z

stack/src/kernel/timesync/timesynckcal-bsdsem.c

+        // Close the semaphore
+        sem_close(instance_l.syncSem);
+        // Unlink the semaphore and timesync shared memory
+        sem_unlink(TIMESYNC_SYNC_BSDSEM);


Please do the semaphore cleanup outside of this function!

Not handled semaphore clean up outside the function, instead the handling is done using goto statements inside the same function itself.

Code duplication: If you do sem_close() and sem_unlink() in timesynckcal_init() if createTimestampShm() fails, you can workaround code duplication!

zelenkaj · 2017-04-18T06:22:13Z

stack/src/kernel/timesync/timesynckcal-bsdsem.c

+        //Close the semaphore
+        sem_close(instance_l.syncSem);
+        //Unlink the semaphore and timesync shared memory
+        sem_unlink(TIMESYNC_SYNC_BSDSEM);


Please do the semaphore cleanup outside of this function!

Not handled semaphore clean up outside the function, instead the handling is done using goto statements inside the same function itself.

Code duplication: If you do sem_close() and sem_unlink() in timesynckcal_init() if createTimestampShm() fails, you can workaround code duplication!

zelenkaj · 2017-04-18T06:22:52Z

stack/src/kernel/timesync/timesynckcal-bsdsem.c

+#if defined(CONFIG_INCLUDE_SOC_TIME_FORWARD)
+    ret = createTimestampShm();
+    if (ret != kErrorOk)
+        return ret;


Do semaphore cleanup here if createTimestampShm() fails!

Semaphore cleanup is already done at createTimestampShm() so it is not required to do it again here.

A function should only clean up those resources which are created in the same function before in case of an error during creation. Of course the clean up in case of an exit might happen in an other function. createTimestampShm() is not creating the semaphore, thus, I would prefer to clean up the semaphore in timesynckcal_init() if createTimestampShm() fails!

zelenkaj · 2017-04-18T06:23:09Z

stack/src/kernel/timesync/timesynckcal-bsdsem.c

+    ret = destroyTimestampShm();
+    if (ret != kErrorOk)
+    {
+        DEBUG_LVL_ERROR_TRACE("%s() timesync shared memory exit failed", __func__); // TODO:Handle ret value in upper layers


drop that TODO

If the user tries to re run the POWERLINK application after an unsuccessful "POWERLINK exit" he will be facing errors due to improper exit, so the previous error handling traces will support the user to debug and progress further.

Hence it is necessary to handle the return value in the upper layers as well, so the TODO is essential here.

I didn't mean to drop the error trace! I completely agree with the scenario you describe that it helps to debug unsuccessful exits.

However, adding to every single exit function a TODO now is not reasonable. Doing this in every exit function would increase the number of TODOs unnecessarily. Instead - if there is a good working way to handle a failing exit except system reset :) - raise an issue to equip all the exit functions with a return value again.

TODO shall be removed and issue shall be raised.

Issue has been raised: #234

zelenkaj · 2017-04-18T06:24:48Z

stack/src/user/timesync/timesyncucal-bsdsem.c

+    ret = destroyTimestampShm();
+    if (ret != kErrorOk)
+    {
+        DEBUG_LVL_ERROR_TRACE("%s() exit failed", __func__); // TODO:Handle ret value in upper layers


drop that TODO

If the user tries to re run the POWERLINK application after an unsuccessful "POWERLINK exit" he will be facing errors due to improper exit, so the previous error handling traces will support the user to debug and progress further.

Hence it is necessary to handle the return value in the upper layers as well, so the TODO is essential here.

same here...

zelenkaj · 2017-04-18T06:25:04Z

stack/src/user/timesync/timesyncucal-bsdsem.c

+        DEBUG_LVL_ERROR_TRACE("%s() Initialization of shared memory failed!\n",
+                              __func__);
+        // Close the semaphore
+        sem_close(instance_l.syncSem);


Please do the semaphore cleanup outside of this function!

Not handled semaphore clean up outside the function, instead the handling is done using goto statements inside the same function itself.

Refer to my comments in https://github.com/OpenAutomationTechnologies/openPOWERLINK_V2/pull/229/files#diff-214cbb4256828fad3d5a5ee993e594b3

zelenkaj · 2017-04-18T06:25:11Z

stack/src/user/timesync/timesyncucal-bsdsem.c

+        // Close the semaphore
+        sem_close(instance_l.syncSem);
+        // Unlink the semaphore and timesync shared memory
+        sem_unlink(TIMESYNC_SYNC_BSDSEM);


Please do the semaphore cleanup outside of this function!

Not handled semaphore clean up outside the function, instead the handling is done using goto statements inside the same function itself.

Refer to my comments in https://github.com/OpenAutomationTechnologies/openPOWERLINK_V2/pull/229/files#diff-214cbb4256828fad3d5a5ee993e594b3

zelenkaj · 2017-04-18T06:26:23Z

stack/src/user/timesync/timesyncucal-bsdsem.c

+#if defined(CONFIG_INCLUDE_SOC_TIME_FORWARD)
+    ret = createTimestampShm();
+    if (ret != kErrorOk)
+        return ret;


Do semaphore cleanup here if createTimestampShm() fails!

Not handled semaphore clean up outside the function, instead the handling is done using goto statements inside the same function itself.

Refer to my comments in https://github.com/OpenAutomationTechnologies/openPOWERLINK_V2/pull/229/files#diff-214cbb4256828fad3d5a5ee993e594b3

zelenkaj suggested changes Apr 18, 2017

View reviewed changes

zelenkaj mentioned this pull request Apr 18, 2017

[FEATURE] Soc timestamp forwarding to application layer in BSDsemaphore #227

Closed

powerlink-team-kalycito mentioned this pull request Apr 27, 2017

[FEATURE] Add SoC time stamp forwarding feature in BSD semaphore design #232

Closed

zelenkaj closed this May 22, 2017

[FEATURE] Add SoC time stamp forwarding feature in BSD semaphore design #229

[FEATURE] Add SoC time stamp forwarding feature in BSD semaphore design #229

Conversation

powerlink-team-kalycito commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelenkaj Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelenkaj Apr 27, 2017 •

edited

Loading