Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ Rewrite ADI Pneumatics class #574

Merged

Conversation

Jerrylum
Copy link
Contributor

@Jerrylum Jerrylum commented May 3, 2023

Summary:

The current design is too complex and buggy. Below is a list of identified anomalies in the Pneumatics class:

Anomaly Category:
Missing, Extraneous, Ambiguous, Inconsistent, Wrong (incorrect fact), Typos (or editorial), Others

Assume get_state get the physical state of the pneumatics

For P = Pneumatics(bool start_extended, bool active_low = false)

# P(false, false)
    extend() -> s = true;
    retract() -> s = true; [W1] should be false
    toggle() -> s = !s;
    get_state() -> false ? state : !state;
                                      ^used
        [W2] if extend() is used, state should be true and the function returns false
             but it is expected to return true as the physical state is true

# P(false, true)
    extend() -> s = false;
    retract() -> s = false; [W3] should be true
    toggle() -> s = !s;
    get_state() -> true ? state : !state;
                            ^used
        [W4] if extend() is used, state should be false and the function returns false
             but it is expected to return true as the physical state is true

# P(true, false)
    extend() -> s = true;
    retract() -> s = true; [W5] should be false
    toggle() -> s = !s;
    get_state() -> false ? state : !state;
                                      ^used
        [W6] if extend() is used, state should be true and the function returns false
             but it is expected to return true as the physical state is true
    
# P(true, true)
    [W7] if extend() is used, state should be false, and it is expected to extend the pneumatics physically
         start_extended is also expected to extend the pneumatics physically when the constructor is called
         but if start_extended set to true, the state also set to true 
    extend() -> s = false;
    retract() -> s = false; [W8] should be true
    toggle() -> s = !s;
    get_state() -> true ? state : !state;
                            ^used
        [W9] if extend() is used, state should be false and the function returns false
             but it is expected to return true as the physical state is true

[A10] state, get_state: is it physical state of the pneumatics or logical state of the pneumatics, what happen if active_low is set to true?
[I11] state and active_low inconsistent if two pneumatics classes defined using the same ADI port

To improve the current design, active_low has been removed and the implementation of some methods has been rewritten.

References (optional):

Bug Report on the PROS Beta Testers Server

@Richard-Stump
Copy link
Contributor

I have updated the implementation to keep the active_low feature. This should hopefully fix the logical error that existed in the original implementation.

Working through the rewritten code, this is the result I got:

	state = false ^ false = false

	extend(): 	state = true
	retract(): 	state = false
	toggle(): 	state = true
	
	is_extended(): returns state ^ extended_is_low = false

p(start_extended = true, extended_is_low = false):
	state = true ^ false = true
	
	extend():	state = true
	retract(): 	state = false
	toggle(): 	state = false
	
	is_extended(): returns state ^ extended_is_low = true
		
p(start_extended = false, extended_is_low = true):
	state = false ^ true = true

	extend(): 	state = false
	retract(): 	state = true
	toggle(): 	state = false
	
	is_extended(): returns state ^ extended_is_low = false
	
p(start_extended = true, extended_is_low = true):
	state = true ^ true = false

	extend(): 	state = false
	retract(): 	state = true
	toggle(): 	state = true
	
	is_extended(): returns state ^ extended_is_low = true

The member function get_state() was renamed to is_extended() to better reflect the fact that it returns the logical state of the pneumatics, not the physical state of the ADI port.

The extend() and retract() functions now return 1 if they are newly extended/retracted, or 0 if they were already in the requested position.

The documentation was also updated to reflect the changes.

@noam987
Copy link
Contributor

noam987 commented May 9, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@noam987 noam987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic Looks good to me and maintains Active low functionality

@noam987 noam987 merged commit 65bcc1a into purduesigbots:develop-pros-4 May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants