CARMA Messenger Integration Test #231
Conversation
| @@ -34,6 +34,8 @@ def __init__(self, sumo_connector, veh_id): | |||
| self.sumo_connector = sumo_connector | |||
| self.sumo_connector.create_stop_veh(self._target_veh_id, self._stop_pos, self._stop_route) | |||
| self.first_time_two_vehicles = True | |||
| self.is_get_closer = False | |||
There was a problem hiding this comment.
Not advocating for any kind of change, but wanted to point out some thoughts for future cases like this:
Generally at a design level, this is where something like a state machine would be good to consider. Right now you've got two variables is_get_closer and is_stopped (and possibly others here) that are really representing a single underlying state of the algorithm. The key is that these two variables are mutually exclusive, as far as I can tell, that the is_get_closer should never be true while is_stopped is true, and vice-versa. So they really encode an aspect of a single actual piece of information, having them as two separate variables like this doesn't really enforce or communicate that relationship between them.
A state machine implementation for something like this could be as simple as identifying the states of the algorithm and then defining an enum with a value per state:
class MoveOverLawState(Enum):
INIT = 1
GET_CLOSER = 2
STOPPING = 3
STOPPED = 4
DEPART = 5
and then tracking it as a single self.state variable. This way you represent that there is really only one state value at a time (rather than two truly independent variables) and represented the total possible state space within that one variable. You could then design the class in a way that uses a switch statement or different methods per state, define transition conditions for each possible transition within the state space, etc.
def process():
if self.state == INIT:
do_init()
if should_transition_to_approaching():
self.state = APPROACHING
elif self.state == APPROACHING:
do_approaching()
if should_transition_to_stopping();
self.state = STOPPING
#etc, etc.
The value of that depends on how complex the design is, probably not necessary for this particular example.
| vehID=veh_id, # Vehicle ID | ||
| edgeID=traci.vehicle.getRoadID(veh_id), # Edge ID where the vehicle stops | ||
| pos=70, # Position (meters) on the edge | ||
| laneIndex=2, # Lane index (e.g., 0 for the first lane) |
There was a problem hiding this comment.
This pos and laneIndex seem hard coded? Will this work for other scenarios?
| @@ -257,7 +267,7 @@ def get_veh_pos(self, veh_id): | |||
|
|
|||
| def create_stop_veh(self, veh_id, end_pos, stop_route): | |||
| try: | |||
| traci.vehicle.add(vehID = veh_id, routeID=stop_route, typeID="car", depart=0, departPos=end_pos) | |||
| traci.vehicle.add(vehID = veh_id, routeID=stop_route, typeID="car", depart=0, departPos=end_pos, departLane='2') | |||
There was a problem hiding this comment.
Same as comment above re hard-coding
| @@ -36,34 +36,35 @@ public class CarmaMessageAmbassador extends CommonMessageAmbassador<CarmaInstanc | |||
| private Thread registrationRxBackgroundThread; | |||
| private CarmaV2xMessageReceiver v2xMessageReceiver; | |||
| private Thread v2xRxBackgroundThread; | |||
| private CarmaInstanceManager carmaInstanceManager; | |||
| private static CarmaInstanceManager carmaInstanceManager = new CarmaInstanceManager(); | |||
There was a problem hiding this comment.
Static is a weird access modifier for this. Is there a case where we need to access this carmaInstanceManager instance variable w/o actually having a constructed instance of the CarmaMessageAmbassador? That'd strike me as kind of off without further context, so I'd be curious to see where that arises if so.
|
PR Details
The update is for the integration testing CXC-118
Related GitHub Issue
Related Jira Key
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: