uniba-swt / swtbahn-cli

A client-server command line interface for the SWTbahn.
GNU General Public License v3.0
7 stars 3 forks source link

Merge Mutex and memory fixes #119

Closed BLuedtke closed 11 months ago

BLuedtke commented 1 year ago

This PR addresses 2 areas:

  1. Mutex: Up until now, the "grabbed_trains_mutex" is sometimes used when reading/writing the grabbedtrains array. This means there is a lot of potential for bugs due to concurrent/conflicting r/w access. Affected areas are mainly the handler... files that use the grabbed_trains array directly.
  2. Memory: The parsers are currently leaking memory all over the place. I analyzed this with address-sanitizer and adjusted them accordingly. Whilst adjusting the parsers, I also streamlined some of the unnecessarily long ifs into if-elses. Additionally, across most handlers, I added checks for the returns of malloc and strdup. I.e., checking if the memory allocation succeeds.

Open TODOs that should be discussed/resolved/moved before merging this PR:

Furthermore, whilst some manual testing has been performed, more rigorous use/testing is needed.

I also suggest to squash commits upon merging, as it's, well, quite a lot of experimental small ones.

BLuedtke commented 1 year ago

The failing build seems to be a regression in the area of config_get_array_string_value, specifically if it's about main_segments. Will investigate.

BLuedtke commented 1 year ago

last_scalar in parser_util was freed a bit too early, causing the parser to act slightly differently in some of the callbacks. Tests now run fine on my machine.

eyip002 commented 1 year ago

Suggested tests:

BLuedtke commented 1 year ago

What does "physical test cases" refer to? There are none for cli, only for bidib. The test script that uses the python client isnt any better than me testing with the python client directly in this case.

Anyway, I just had a long test session. Tested all web pages (normal + game) and the full python client. test notes 2023-08-04.md

Only one issue was querying the reverser state. need to check that in more detail and compare against other branches.

Interesting: I think I found a much simpler reason for why we had trains going the wrong way, but I need to do some more checks to confirm...

BLuedtke commented 1 year ago

Alright, I can't get the reverser state if no train is sitting on the Kehrschleife. But this is the same as on the master branch. If a train is on there, I get the info as intended. So that works, and concludes my testing.

eyip002 commented 1 year ago

Should probably make physical test cases based on sample_test_script.sh for switching points and signals, and running a train.

The reverser query behaviour doesn't sound normal. Are you able to get the monitor to print "on" and "off" for the reverser state?

Is BiDiB Wizard able to read the reverser state?

2021-08-04-185540_1920x1080_scrot

BLuedtke commented 1 year ago

I will open a new issue for the reverser problem, as it happens on the master branch too.

BLuedtke commented 1 year ago

The other problem I found is that automatic route driving get's the direction wrong in a very specific situation with the Kehrschleife. Maybe the two problem are interlinked.